improved cloudflare worker support #1274
Conversation
cpinn
force-pushed
the
caitlin/externalize
branch
from
b716030 to
8d2e5e2
Compare
cpinn
changed the title
…p to avoid removing the prefix
| environment === "cloudflare-worker-browser-no-compat" || | ||
| environment === "cloudflare-worker-browser-node-compat" || | ||
| environment === "nextjs-edge-runtime" || | ||
| environment === "vite-react-hono") && |
There was a problem hiding this comment.
not excited for this. i think instead of having the shared tests we should make the nunjucks/rendering be specific per smoke test. this way we avoid this complexity and makes maintenance easier.
There was a problem hiding this comment.
I haven't changed this because I am actually still a bit annoyed about removing the support from all these places. I get we need to do something for vite but I am still not necessarily on board with dropping everything so I didn't really want to put effort into doing something there until we 100% decided that was the stance we wanted to take.
Same with some of the other cleanup.
| import * as braintrust from "braintrust/browser"; | ||
| import { createWorker } from "./worker"; | ||
|
|
||
| export default createWorker(braintrust, "cloudflare-worker-browser-no-compat"); |
There was a problem hiding this comment.
it is not at the moment with the way the files are setup.
| import * as braintrust from "braintrust"; | ||
| import { createWorker } from "./worker"; | ||
|
|
||
| export default createWorker(braintrust, "cloudflare-worker-node-node-compat"); |
| runImportVerificationTests, | ||
| runPromptTemplatingTests, | ||
| type TestResult, | ||
| } from "../../../shared/dist/index.mjs"; |
There was a problem hiding this comment.
just realized we're importing from shared/dist. shouldn't we let each environment decide if it will import from cjs or mjs?
| const adapters = await setupTestEnvironment({ | ||
| initLogger, | ||
| testingExports: _exportsForTestingOnly, | ||
| canUseFileSystem: false, |
There was a problem hiding this comment.
looks like this should be true? https://developers.cloudflare.com/workers/runtime-apis/nodejs/fs/
| "description": "Smoke test for Braintrust Node build in Deno runtime", | ||
| "private": true, | ||
| "scripts": { | ||
| "test": "deno test --no-config --import-map=./deno.json --lock=deno.lock --allow-env --allow-read=.,../build --allow-net shared_suite_test.ts span_test.ts" |
| } from "../../../../shared/dist/index.mjs"; | ||
|
|
||
| import * as braintrust from "braintrust"; | ||
| import * as braintrust from "braintrust/browser"; |
There was a problem hiding this comment.
we might need two versions. one that is browser and one that is node.
There was a problem hiding this comment.
maybe we will not learn something new, though 🤔 but i know this test the particular interesting one is the dev mode
There was a problem hiding this comment.
The node one will always fail. Nunjucks and vite do not work together. if you use vite you need to use browser.
There was a problem hiding this comment.
| "name": "braintrust-vite-react-hono-smoke-test", | ||
| "main": "./src/worker/index.ts", | ||
| "compatibility_date": "2025-01-15", | ||
| "compatibility_flags": ["nodejs_compat"] |
There was a problem hiding this comment.
same here did we want to try without compat? maybe the same results
There was a problem hiding this comment.
interesting as well the min. 2025-01-15
There was a problem hiding this comment.
we can. I did that in the other tests.
I do wonder how useful it is without the compat in general since then you lose all the hierarchy. I think that would be a pretty annoying span experience.
| return process.env[name]; | ||
| }; | ||
|
|
||
| iso.renderNunjucksString = () => { |
There was a problem hiding this comment.
did we still need nunjucks-env.browser.ts and nunjucks-utils.browser.ts?
There was a problem hiding this comment.
no they were from some issue I thought I was running into with the deno build and I had been waiting to see if this build finally passed.
| import { _internalSetInitialState } from "./logger"; | ||
| import { promisify } from "util"; | ||
| import * as zlib from "zlib"; | ||
| import { renderNunjucksString as nunjucksRender } from "./template/nunjucks-env"; |
There was a problem hiding this comment.
something to consider for the future (or perhaps the ai is quick to do this).
i previously added a lint rule to disallow import from cli in the sdk/js.
what if we had a sdk/js/node and sdk/js/browser ... we then move files in two each of those. we could say that nothing is allowed to depend on sdk/js/node/** unless the it stems from node.ts, and same with browser.ts
| entry: ["src/browser.ts"], | ||
| format: ["cjs", "esm"], | ||
| outDir: "dist", | ||
| removeNodeProtocol: false, |
There was a problem hiding this comment.
@ibolmo should this be false? it was false when I cherry picked the change but I was confused why we would want it false for the browser build.
Problem
Cloudflare Workers are encountering compatibility issues with certain dependencies in the Braintrust SDK. These issues surface particularly in restrictive runtime environments and how some dependencies behave in Vite-based builds.
Changes
Maintain this existing build structure:
braintrust (CJS) -> node
braintrust (ESM) -> node
braintrust/browser -> no cli, no filesystem, no git, no nunjucks