Repository navigation
feat(sdk): make typescript/node follow best practices (e.g. ESM) - #3648
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request migrates the Node.js SDK from CommonJS+Jest to ESM+Vitest. The package now publishes from a ChangesNode.js SDK ESM + Vitest migration
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5fc86535bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "version": "0.0.0-beta", | ||
| "main": "index.js", | ||
| "types": "index.d.ts", | ||
| "type": "module", |
There was a problem hiding this comment.
Compile the entrypoint as ESM before marking it module
With this package set to "type": "module", Node treats the emitted index.js as ESM, but the unchanged tsconfig.json still emits CommonJS and the checked-in/generated entrypoint uses exports and require(...). Any consumer importing this package now aborts before loading the bridge (e.g. Node reports ReferenceError: exports is not defined in ES module scope for index.js), so the package entrypoint is unusable until the TS output is actually converted to ESM or the CommonJS files are renamed/exported as .cjs.
Useful? React with 👍 / 👎.
| "ts-jest": "^29.1.0", | ||
| "typescript": "^5.4.5" | ||
| "typescript": "^5.4.5", | ||
| "vitest": "^3.2.3" |
There was a problem hiding this comment.
Update the lockfiles with the dependency swap
The switch from Jest to Vitest is not reflected in the lockfiles: the root importer and the package-local pnpm-lock.yaml still list jest, ts-jest, and @types/jest but no vitest for this package. I checked pnpm --dir baml_language/sdks/nodejs/bridge_nodejs install --frozen-lockfile --ignore-scripts, and pnpm fails with ERR_PNPM_OUTDATED_LOCKFILE; any CI or release path using frozen lockfile installs will fail before build/test starts.
Useful? React with 👍 / 👎.
| "build:proto": "node node_modules/protobufjs-cli/bin/pbjs -t static-module -w commonjs -p ../../../crates/bridge_ctypes/types -o typescript_src/proto/baml_cffi.js ../../../crates/bridge_ctypes/types/baml_core/cffi/v1/baml_inbound.proto ../../../crates/bridge_ctypes/types/baml_core/cffi/v1/baml_outbound.proto && node node_modules/protobufjs-cli/bin/pbts -o typescript_src/proto/baml_cffi.d.ts typescript_src/proto/baml_cffi.js", | ||
| "build:napi-debug": "napi build --js ./native.js --dts ./native.d.ts --platform && cp native.d.ts typescript_src/native.d.ts", | ||
| "build:napi-release": "napi build --js ./native.js --dts ./native.d.ts --platform --release && cp native.d.ts typescript_src/native.d.ts", | ||
| "build:proto": "node node_modules/protobufjs-cli/bin/pbjs -t static-module -w commonjs -p ../../../crates/bridge_ctypes/types -o typescript_src/proto/baml_cffi.cjs ../../../crates/bridge_ctypes/types/baml_core/cffi/v1/baml_inbound.proto ../../../crates/bridge_ctypes/types/baml_core/cffi/v1/baml_outbound.proto", |
There was a problem hiding this comment.
Keep protobuf output aligned with its consumers
This now generates only typescript_src/proto/baml_cffi.cjs, but the rest of this package still imports/copies/tags baml_cffi as .js/.d.ts (typescript_src/proto.ts, copy-proto.js, and tag-generated-files.js). On a clean generation or after any proto change, the freshly generated .cjs is never the file loaded or copied, and no updated declaration file is produced, so the build either uses stale checked-in protobuf artifacts or fails if those artifacts are absent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/sdks/nodejs/bridge_nodejs/package.json`:
- Around line 34-35: The package scripts build:napi-debug and build:napi-release
use the Unix-only cp command which breaks on Windows; replace the shell cp usage
with a cross-platform solution (e.g. a Node-based copy or a CLI like cpy/cpx) so
pnpm scripts run on all platforms. Update the two scripts to perform the same
file copy of native.d.ts into typescript_src/native.d.ts using a Node-compatible
approach and ensure the change preserves the existing flags (--js, --esm, --dts,
--platform, --release).
- Line 33: The build step changed the generated filename to baml_cffi.cjs but
the copy script (typescript_src/copy-proto.js) and runtime import
(require("./proto/baml_cffi")) still expect baml_cffi.js and typings, causing
build failures; fix by either (A) updating the package.json "build:proto" script
to emit the legacy artifacts (typescript_src/proto/baml_cffi.js plus a .d.ts via
pbts) so copy-proto.js and require("./proto/baml_cffi") continue to work, or (B)
keep the .cjs output and update copy-proto.js to copy baml_cffi.cjs and
remove/adjust .d.ts expectations and change runtime imports to
require("./proto/baml_cffi.cjs"); pick one option and apply consistently across
build:proto, typescript_src/copy-proto.js, and any runtime require sites.
- Around line 4-12: package.json declares ESM ("type": "module" and
exports.import -> "./index.js") but the generated runtime files (index.js,
native.js) and proto artifacts are CommonJS and the build scripts are
inconsistent/portable; fix by either switching package.json to CommonJS (remove
"type": "module" and add an exports.require entry pointing to "./index.js") OR
regenerate/emit ESM-compatible build outputs for index.js/native.js and adjust
exports.import accordingly; update the proto build pipeline so build:proto emits
typescript_src/proto/baml_cffi.js (or change typescript_src/copy-proto.js and
typescript_src/tag-generated-files.js to expect .cjs) to keep artifact names
consistent; and replace shell cp in build:napi-debug/release scripts with a
cross-platform file copy (use a Node script or an npm cross-copy utility) so
Windows builds work.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2af594cf-6e30-42eb-97c3-bab92b94771f
📒 Files selected for processing (1)
baml_language/sdks/nodejs/bridge_nodejs/package.json
Binary size checks passed✅ 7 passed
Generated by |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
baml_language/sdk_tests/harness_setup/src/typescript_node.rs (1)
68-76:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix stale symlink rationale for
preserveSymlinksinTSCONFIG_JSONdocs
The comment aboveTSCONFIG_JSON(lines ~67-75) sayscustomizable/*.test.tsare symlinked and thatpreserveSymlinks: trueis load-bearing, but the TypeScript harness now copies the entirecustomizable/overlay intogenerated/viacopy_customizable(usesfs::copy, lines ~186-193 andcopy_customizableinharness_setup/src/lib.rs). Update the comment to match the current copy behavior (and clarify whypreserveSymlinksis still set totrue, if it is required for other symlinks).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdk_tests/harness_setup/src/typescript_node.rs` around lines 68 - 76, Update the comment above TSCONFIG_JSON to remove the stale claim that customizable/*.test.ts are symlinked and instead note that copy_customizable (which uses fs::copy) now copies the entire customizable overlay into generated/, so tests are not symlinked; also state whether preserveSymlinks: true remains necessary for any other symlinked assets (or remove that justification if not), and reference TSCONFIG_JSON and copy_customizable in the comment so future readers can find the relevant behavior.
🧹 Nitpick comments (1)
baml_language/sdk_tests/harness_setup/src/templates/package.json (1)
1-17: 💤 Low valueConsider adding Node.js engine constraint.
The configuration uses
"type": "module", ES2022, andnodenextmodule resolution, which require Node.js 16.0.0 or later. Adding anenginesfield would document this requirement and prevent runtime issues.📦 Suggested engine constraint
"devDependencies": { "`@types/node`": "^20.0.0", "typescript": "^5.4.5", "vitest": "^3.2.3" + }, + "engines": { + "node": ">=16.0.0" } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdk_tests/harness_setup/src/templates/package.json` around lines 1 - 17, Add a Node.js engine constraint to package.json to document and enforce the minimum Node version required by "type": "module" and ES2022/nodenext resolution; update the package manifest (the object containing "name", "version", "type", "scripts", "dependencies", "devDependencies") to include an "engines" field (e.g., "engines": { "node": ">=16.0.0" }) so package managers and users are informed of the runtime requirement.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@baml_language/sdk_tests/crates/typescript_node/docstrings_etc/customizable/main.test.ts`:
- Around line 5-12: The test uses __dirname inside docsSource but the file is
ESM so __dirname is undefined; update docsSource to compute the directory from
import.meta.url (e.g. use fileURLToPath(import.meta.url) and path.dirname) and
replace join(__dirname, "baml_sdk", "docs", "index.ts") accordingly; also add
the necessary imports (fileURLToPath from "url" and dirname or dirname from
"path" alongside join) and ensure docsSource reads the resolved path.
In
`@baml_language/sdks/nodejs/bridge_nodejs/typescript_src/tag-generated-files.js`:
- Around line 29-30: The replacement that rewrites `import Long =
require("long");` to `import Long from "long";` in the branch that handles
filenames ending with `baml_cffi.d.ts` must be removed or guarded so the `.d.ts`
is left with the original `import Long = require("long");`; update the code in
tag-generated-files.js (the if (f.endsWith('baml_cffi.d.ts')) branch and the c =
c.replaceAll(...) call) to stop performing that rewrite so the generated
declaration keeps the `export =`-style import that consumers without
esModuleInterop/allowSyntheticDefaultImports require.
---
Outside diff comments:
In `@baml_language/sdk_tests/harness_setup/src/typescript_node.rs`:
- Around line 68-76: Update the comment above TSCONFIG_JSON to remove the stale
claim that customizable/*.test.ts are symlinked and instead note that
copy_customizable (which uses fs::copy) now copies the entire customizable
overlay into generated/, so tests are not symlinked; also state whether
preserveSymlinks: true remains necessary for any other symlinked assets (or
remove that justification if not), and reference TSCONFIG_JSON and
copy_customizable in the comment so future readers can find the relevant
behavior.
---
Nitpick comments:
In `@baml_language/sdk_tests/harness_setup/src/templates/package.json`:
- Around line 1-17: Add a Node.js engine constraint to package.json to document
and enforce the minimum Node version required by "type": "module" and
ES2022/nodenext resolution; update the package manifest (the object containing
"name", "version", "type", "scripts", "dependencies", "devDependencies") to
include an "engines" field (e.g., "engines": { "node": ">=16.0.0" }) so package
managers and users are informed of the runtime requirement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e9c9766b-e61a-4b61-813a-8a0f76526dc7
⛔ Files ignored due to path filters (13)
baml_language/sdks/nodejs/bridge_nodejs/ctx_manager.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/define_function.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/errors.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/exit_hook.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/index.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/index.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlbaml_language/sdks/nodejs/bridge_nodejs/proto.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/stream.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/stream.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/typemap.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/typemap.js.mapis excluded by!**/*.map
📒 Files selected for processing (74)
baml_language/sdk_tests/README.mdbaml_language/sdk_tests/crates/typescript_node/docstrings_etc/customizable/main.test.tsbaml_language/sdk_tests/crates/typescript_node/function_calls/customizable/methods_on_classes.test.tsbaml_language/sdk_tests/crates/typescript_node/function_calls/customizable/stdlib_entrypoints.test.tsbaml_language/sdk_tests/crates/typescript_node/llm_functions/customizable/main.test.tsbaml_language/sdk_tests/crates/typescript_node/setup.shbaml_language/sdk_tests/crates/typescript_node/src/lib.rsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/main.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_aliases.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_class_refs.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_complex_models.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_enums.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_forward_refs.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_generics.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_handles.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_lists.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_literals.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_maps.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_media.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_optional.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_primitives.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_recursion.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_routing.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_streams.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_symbol_collisions.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_unions.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_void.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/test_generic.test.tsbaml_language/sdk_tests/harness_setup/src/templates/package.jsonbaml_language/sdk_tests/harness_setup/src/templates/tsconfig.jsonbaml_language/sdk_tests/harness_setup/src/typescript_node.rsbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.d.tsbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.jsbaml_language/sdks/nodejs/bridge_nodejs/define_function.jsbaml_language/sdks/nodejs/bridge_nodejs/errors.jsbaml_language/sdks/nodejs/bridge_nodejs/exit_hook.jsbaml_language/sdks/nodejs/bridge_nodejs/index.d.tsbaml_language/sdks/nodejs/bridge_nodejs/index.jsbaml_language/sdks/nodejs/bridge_nodejs/native.jsbaml_language/sdks/nodejs/bridge_nodejs/package.jsonbaml_language/sdks/nodejs/bridge_nodejs/proto.jsbaml_language/sdks/nodejs/bridge_nodejs/proto/baml_cffi.d.tsbaml_language/sdks/nodejs/bridge_nodejs/proto/baml_cffi.jsbaml_language/sdks/nodejs/bridge_nodejs/stream.d.tsbaml_language/sdks/nodejs/bridge_nodejs/stream.jsbaml_language/sdks/nodejs/bridge_nodejs/tests/call_function.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/host_callable.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/run_vitest.rsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_collector.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_decode_call_result.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_decode_handle.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_engine.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_media.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_stream.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_tracing.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_typemap.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tsconfig.jsonbaml_language/sdks/nodejs/bridge_nodejs/typemap.jsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/copy-proto.jsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/ctx_manager.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/define_function.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/exit_hook.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/index.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.jsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/stream.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/tag-generated-files.jsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/typemap.tsbaml_language/sdks/nodejs/bridge_nodejs/vitest.config.tsbaml_language/sdks/nodejs/codegen_nodejs/src/emit/typemap_file.rsbaml_language/sdks/nodejs/codegen_nodejs/src/leaf.rsbaml_language/sdks/nodejs/codegen_nodejs/src/lib.rsbaml_language/sdks/nodejs/codegen_nodejs/src/routing.rs
✅ Files skipped from review due to trivial changes (22)
- baml_language/sdks/nodejs/bridge_nodejs/tests/test_media.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/exit_hook.ts
- baml_language/sdks/nodejs/bridge_nodejs/stream.d.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/test_generic.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/tests/test_engine.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/ctx_manager.d.ts
- baml_language/sdk_tests/crates/typescript_node/src/lib.rs
- baml_language/sdks/nodejs/bridge_nodejs/tests/test_tracing.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/tests/call_function.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/stream.ts
- baml_language/sdks/nodejs/bridge_nodejs/tests/test_collector.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/vitest.config.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts
- baml_language/sdk_tests/crates/typescript_node/setup.sh
- baml_language/sdks/nodejs/codegen_nodejs/src/routing.rs
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.d.ts
- baml_language/sdks/nodejs/bridge_nodejs/proto/baml_cffi.d.ts
- baml_language/sdks/nodejs/bridge_nodejs/index.d.ts
- baml_language/sdk_tests/README.md
- baml_language/sdks/nodejs/bridge_nodejs/ctx_manager.js
- baml_language/sdks/nodejs/bridge_nodejs/index.js
- baml_language/sdks/nodejs/bridge_nodejs/proto.js
| import { describe, it, expect } from "vitest"; | ||
| import { readFileSync } from "node:fs"; | ||
| import { join } from "node:path"; | ||
|
|
||
| import { Doc, Note, Priority, Sentiment } from "./baml_sdk/docs"; | ||
| import { Doc, Note, Priority, Sentiment } from "./baml_sdk/docs/index.js"; | ||
|
|
||
| const docsSource = () => | ||
| readFileSync(join(__dirname, "baml_sdk", "docs", "index.ts"), "utf8").replace(/\r\n/g, "\n"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Test/module config =="
fd -a 'package.json|tsconfig.json|vitest.config.ts' baml_language/sdk_tests/crates/typescript_node baml_language/sdk_tests/harness_setup \
| xargs -I{} sh -c 'echo "--- {} ---"; sed -n "1,200p" "{}"; echo'
echo "== __dirname usages in TypeScript Node SDK tests =="
rg -n --type=ts '\b__dirname\b' baml_language/sdk_tests/crates/typescript_nodeRepository: BoundaryML/baml
Length of output: 1293
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for any __dirname polyfills/workarounds =="
rg -n --type=ts --type=js '\b__dirname\b' baml_language/sdk_tests/crates/typescript_node baml_language/sdk_tests/harness_setup
echo "== Check for vitest/ts-node/esm config that might define __dirname =="
fd -a 'vitest.config.ts|vitest.config.js|vitest.workspace.ts|tsconfig*.json|node*.json' baml_language/sdk_tests/harness_setup baml_language/sdk_tests/crates/typescript_node \
| xargs -I{} sh -c 'echo "--- {} ---"; sed -n "1,200p" "{}"'Repository: BoundaryML/baml
Length of output: 890
Fix ESM test by removing __dirname usage
baml_language/sdk_tests/crates/typescript_node/docstrings_etc/customizable/main.test.ts is executed as ESM (type: "module", module: "nodenext"), so __dirname is undefined in the test module scope and the test will fail before assertions run.
Suggested fix
import { describe, it, expect } from "vitest";
import { readFileSync } from "node:fs";
import { join } from "node:path";
+import { fileURLToPath } from "node:url";
import { Doc, Note, Priority, Sentiment } from "./baml_sdk/docs/index.js";
+
+const __dirname = fileURLToPath(new URL(".", import.meta.url));
const docsSource = () =>
readFileSync(join(__dirname, "baml_sdk", "docs", "index.ts"), "utf8").replace(/\r\n/g, "\n");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { describe, it, expect } from "vitest"; | |
| import { readFileSync } from "node:fs"; | |
| import { join } from "node:path"; | |
| import { Doc, Note, Priority, Sentiment } from "./baml_sdk/docs"; | |
| import { Doc, Note, Priority, Sentiment } from "./baml_sdk/docs/index.js"; | |
| const docsSource = () => | |
| readFileSync(join(__dirname, "baml_sdk", "docs", "index.ts"), "utf8").replace(/\r\n/g, "\n"); | |
| import { describe, it, expect } from "vitest"; | |
| import { readFileSync } from "node:fs"; | |
| import { join } from "node:path"; | |
| import { fileURLToPath } from "node:url"; | |
| import { Doc, Note, Priority, Sentiment } from "./baml_sdk/docs/index.js"; | |
| const __dirname = fileURLToPath(new URL(".", import.meta.url)); | |
| const docsSource = () => | |
| readFileSync(join(__dirname, "baml_sdk", "docs", "index.ts"), "utf8").replace(/\r\n/g, "\n"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@baml_language/sdk_tests/crates/typescript_node/docstrings_etc/customizable/main.test.ts`
around lines 5 - 12, The test uses __dirname inside docsSource but the file is
ESM so __dirname is undefined; update docsSource to compute the directory from
import.meta.url (e.g. use fileURLToPath(import.meta.url) and path.dirname) and
replace join(__dirname, "baml_sdk", "docs", "index.ts") accordingly; also add
the necessary imports (fileURLToPath from "url" and dirname or dirname from
"path" alongside join) and ensure docsSource reads the resolved path.
| if (f.endsWith('baml_cffi.d.ts')) { | ||
| c = c.replaceAll('import Long = require("long");', 'import Long from "long";'); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does TypeScript allow import Long from "long"in a.d.tswhen thelongpackage usesexport = Long, without esModuleInteroporallowSyntheticDefaultImports? Please cite TypeScript and/or long package documentation.
💡 Result:
No, TypeScript does not allow import Long from "long" in a .d.ts file (or any TypeScript file) when the long package uses export = Long and the esModuleInterop and allowSyntheticDefaultImports flags are disabled [1][2]. When a module uses the export = syntax, it follows the CommonJS model, which TypeScript specifically requires to be imported using the import ... = require(...) syntax [3][4]. Using the standard import Long from "long" syntax to import an export = module is incompatible with TypeScript's module resolution and type-checking rules when those flags are disabled [1][5][6][2]. Specifically: 1. The export = syntax is a TypeScript-specific construct used to model traditional CommonJS module.exports [4][7]. 2. Without esModuleInterop or allowSyntheticDefaultImports, TypeScript does not permit treating a CommonJS/UMD module as an ES module for the purpose of default imports [5][6]. Attempting to do so will result in an error such as TS2497 (or similar, depending on the environment), indicating that the module can only be imported using require [1][2]. 3. To correctly import such a module, you must use: import Long = require("long"); [1][4]. This requirement is well-documented in TypeScript's module reference, which explains that export = is intended to be used with the corresponding import = require() syntax [4][7]. While esModuleInterop was introduced to relax these restrictions by creating synthetic default imports for CommonJS modules, disabling it reverts TypeScript to its strict interpretation of module compatibility [5][6].
Citations:
- 1: Unable import in TypeScript since v5+ dcodeIO/long.js#109
- 2: https://stackoverflow.com/questions/37651495/difference-between-import-from-and-import-require-in-typescript
- 3: https://www.typescriptlang.org/docs/handbook/2/modules.html
- 4: https://github.com/microsoft/TypeScript-Handbook/blob/master/pages/Modules.md
- 5: https://www.typescriptlang.org/tsconfig/esModuleInterop
- 6: https://www.typescriptlang.org/tsconfig/allowSyntheticDefaultImports.html
- 7: https://typescriptdocs.com/modules-reference/reference.html
Don’t rewrite the long import in baml_cffi.d.ts
In baml_language/sdks/nodejs/bridge_nodejs/typescript_src/tag-generated-files.js, changing import Long = require("long"); to import Long from "long"; makes the published .d.ts rely on esModuleInterop/allowSyntheticDefaultImports. Since long uses export =, TypeScript requires import ... = require(...) for correct typing when those interop flags aren’t enabled, so this can break consumer type-checking.
Suggested fix
- if (f.endsWith('baml_cffi.d.ts')) {
- c = c.replaceAll('import Long = require("long");', 'import Long from "long";');
- }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@baml_language/sdks/nodejs/bridge_nodejs/typescript_src/tag-generated-files.js`
around lines 29 - 30, The replacement that rewrites `import Long =
require("long");` to `import Long from "long";` in the branch that handles
filenames ending with `baml_cffi.d.ts` must be removed or guarded so the `.d.ts`
is left with the original `import Long = require("long");`; update the code in
tag-generated-files.js (the if (f.endsWith('baml_cffi.d.ts')) branch and the c =
c.replaceAll(...) call) to stop performing that rewrite so the generated
declaration keeps the `export =`-style import that consumers without
esModuleInterop/allowSyntheticDefaultImports require.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
baml_language/sdk_tests/harness_runner/src/lib.rs (1)
168-197: ⚡ Quick winAdd unit tests for the new import-specifier parser.
import_from_specifierand the.js-suffix assertion are pure helper logic now sitting on the critical ESM verification path. A few module-local tests would lock down the accepted import/export shapes without depending on full fixture generation. Based on learnings,**/*.rs: Prefer writing Rust unit tests over integration tests where possible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdk_tests/harness_runner/src/lib.rs` around lines 168 - 197, Add unit tests for the import specifier parsing and the `.js` suffix assertion by creating module-local Rust unit tests that exercise import_from_specifier and the logic that checks for `.js` suffix (the helpers used on the ESM verification path). Write tests covering typical import/export forms (single- and double-quoted specifiers, leading/trailing whitespace, no " from " cases, non-string specifiers) and cases asserting `.js` suffix handling (with and without `.js`, uppercase/lowercase, edge cases like empty specifier). Put tests in the same file using #[cfg(test)] mod tests so they run as unit tests and reference import_from_specifier and the suffix-check code directly to lock down accepted shapes without requiring fixture generation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@baml_language/sdk_tests/harness_runner/src/lib.rs`:
- Around line 168-197: Add unit tests for the import specifier parsing and the
`.js` suffix assertion by creating module-local Rust unit tests that exercise
import_from_specifier and the logic that checks for `.js` suffix (the helpers
used on the ESM verification path). Write tests covering typical import/export
forms (single- and double-quoted specifiers, leading/trailing whitespace, no "
from " cases, non-string specifiers) and cases asserting `.js` suffix handling
(with and without `.js`, uppercase/lowercase, edge cases like empty specifier).
Put tests in the same file using #[cfg(test)] mod tests so they run as unit
tests and reference import_from_specifier and the suffix-check code directly to
lock down accepted shapes without requiring fixture generation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b08263cf-9db0-406b-8f09-4dd1d1fc68a6
⛔ Files ignored due to path filters (13)
baml_language/sdks/nodejs/bridge_nodejs/ctx_manager.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/define_function.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/errors.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/exit_hook.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/index.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/index.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlbaml_language/sdks/nodejs/bridge_nodejs/proto.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/stream.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/stream.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/typemap.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/typemap.js.mapis excluded by!**/*.map
📒 Files selected for processing (76)
baml_language/sdk_tests/README.mdbaml_language/sdk_tests/crates/typescript_node/docstrings_etc/customizable/main.test.tsbaml_language/sdk_tests/crates/typescript_node/function_calls/customizable/methods_on_classes.test.tsbaml_language/sdk_tests/crates/typescript_node/function_calls/customizable/stdlib_entrypoints.test.tsbaml_language/sdk_tests/crates/typescript_node/llm_functions/customizable/main.test.tsbaml_language/sdk_tests/crates/typescript_node/setup.shbaml_language/sdk_tests/crates/typescript_node/src/lib.rsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/main.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_aliases.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_class_refs.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_complex_models.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_enums.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_forward_refs.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_generics.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_handles.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_lists.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_literals.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_maps.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_media.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_optional.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_primitives.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_recursion.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_routing.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_streams.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_symbol_collisions.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_unions.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_void.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/test_generic.test.tsbaml_language/sdk_tests/harness_runner/src/lib.rsbaml_language/sdk_tests/harness_setup/src/templates/package.jsonbaml_language/sdk_tests/harness_setup/src/templates/tsconfig.jsonbaml_language/sdk_tests/harness_setup/src/typescript_node.rsbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.d.tsbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.jsbaml_language/sdks/nodejs/bridge_nodejs/define_function.jsbaml_language/sdks/nodejs/bridge_nodejs/errors.jsbaml_language/sdks/nodejs/bridge_nodejs/exit_hook.jsbaml_language/sdks/nodejs/bridge_nodejs/index.d.tsbaml_language/sdks/nodejs/bridge_nodejs/index.jsbaml_language/sdks/nodejs/bridge_nodejs/native.jsbaml_language/sdks/nodejs/bridge_nodejs/package.jsonbaml_language/sdks/nodejs/bridge_nodejs/proto.jsbaml_language/sdks/nodejs/bridge_nodejs/proto/baml_cffi.d.tsbaml_language/sdks/nodejs/bridge_nodejs/proto/baml_cffi.jsbaml_language/sdks/nodejs/bridge_nodejs/stream.d.tsbaml_language/sdks/nodejs/bridge_nodejs/stream.jsbaml_language/sdks/nodejs/bridge_nodejs/tests/call_function.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/host_callable.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/run_vitest.rsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_collector.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_decode_call_result.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_decode_handle.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_engine.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_media.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_stream.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_tracing.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_typemap.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tsconfig.jsonbaml_language/sdks/nodejs/bridge_nodejs/typemap.jsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/copy-native-dts.jsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/copy-proto.jsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/ctx_manager.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/define_function.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/exit_hook.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/index.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.jsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/stream.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/tag-generated-files.jsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/typemap.tsbaml_language/sdks/nodejs/bridge_nodejs/vitest.config.tsbaml_language/sdks/nodejs/codegen_nodejs/src/emit/typemap_file.rsbaml_language/sdks/nodejs/codegen_nodejs/src/leaf.rsbaml_language/sdks/nodejs/codegen_nodejs/src/lib.rsbaml_language/sdks/nodejs/codegen_nodejs/src/routing.rs
✅ Files skipped from review due to trivial changes (16)
- baml_language/sdks/nodejs/bridge_nodejs/tests/test_collector.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/tests/test_engine.test.ts
- baml_language/sdk_tests/crates/typescript_node/src/lib.rs
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_handles.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/exit_hook.ts
- baml_language/sdk_tests/README.md
- baml_language/sdks/nodejs/bridge_nodejs/tests/test_tracing.test.ts
- baml_language/sdk_tests/crates/typescript_node/setup.sh
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_complex_models.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/stream.d.ts
- baml_language/sdks/nodejs/bridge_nodejs/tests/test_decode_handle.test.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_media.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_streams.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/ctx_manager.js
- baml_language/sdks/nodejs/codegen_nodejs/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (40)
- baml_language/sdks/nodejs/bridge_nodejs/ctx_manager.d.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_primitives.test.ts
- baml_language/sdk_tests/crates/typescript_node/function_calls/customizable/methods_on_classes.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/tsconfig.json
- baml_language/sdks/nodejs/codegen_nodejs/src/routing.rs
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_generics.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/typemap.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/stream.ts
- baml_language/sdk_tests/crates/typescript_node/llm_functions/customizable/main.test.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/test_generic.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/copy-proto.js
- baml_language/sdk_tests/crates/typescript_node/docstrings_etc/customizable/main.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/ctx_manager.ts
- baml_language/sdks/nodejs/bridge_nodejs/proto/baml_cffi.d.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/index.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.d.ts
- baml_language/sdks/nodejs/bridge_nodejs/index.d.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/main.test.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_void.test.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_forward_refs.test.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_routing.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/tag-generated-files.js
- baml_language/sdks/nodejs/bridge_nodejs/tests/host_callable.test.ts
- baml_language/sdk_tests/harness_setup/src/templates/tsconfig.json
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_aliases.test.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_class_refs.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/vitest.config.ts
- baml_language/sdk_tests/harness_setup/src/typescript_node.rs
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_maps.test.ts
- baml_language/sdks/nodejs/codegen_nodejs/src/emit/typemap_file.rs
- baml_language/sdks/nodejs/bridge_nodejs/native.js
- baml_language/sdks/nodejs/bridge_nodejs/tests/test_typemap.test.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_literals.test.ts
- baml_language/sdks/nodejs/codegen_nodejs/src/leaf.rs
- baml_language/sdk_tests/harness_setup/src/templates/package.json
- baml_language/sdks/nodejs/bridge_nodejs/tests/run_vitest.rs
- baml_language/sdks/nodejs/bridge_nodejs/errors.js
- baml_language/sdks/nodejs/bridge_nodejs/define_function.js
- baml_language/sdks/nodejs/bridge_nodejs/index.js
- baml_language/sdks/nodejs/bridge_nodejs/proto.js
There was a problem hiding this comment.
♻️ Duplicate comments (1)
baml_language/sdk_tests/crates/typescript_node/docstrings_etc/customizable/main.test.ts (1)
12-12:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
__dirnameis undefined in ESM context.This test runs as ESM (
type: "module",module: "nodenext"), so__dirnameis not available. The test will fail at runtime whendocsSource()attempts to read the file.🔧 Proposed fix
import { describe, it, expect } from "vitest"; import { readFileSync } from "node:fs"; import { join } from "node:path"; +import { fileURLToPath } from "node:url"; import { Doc, Note, Priority, Sentiment } from "./baml_sdk/docs/index.js"; + +const __dirname = fileURLToPath(new URL(".", import.meta.url)); const docsSource = () => readFileSync(join(__dirname, "baml_sdk", "docs", "index.ts"), "utf8").replace(/\r\n/g, "\n");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdk_tests/crates/typescript_node/docstrings_etc/customizable/main.test.ts` at line 12, The test uses __dirname (in the readFileSync call inside docsSource() in main.test.ts) which is undefined in ESM; replace that usage by deriving the directory from import.meta.url (e.g. use fileURLToPath(import.meta.url) and path.dirname or new URL('.', import.meta.url)) and update the readFileSync(join(...)) call to use that computed directory; also add the necessary import (fileURLToPath from 'url' or use new URL) at the top of the test so the file read works in ESM.
🧹 Nitpick comments (1)
baml_language/sdks/nodejs/bridge_nodejs/package.json (1)
47-47: Check vitest dependency (^3.2.3) for stability
[email protected]is published (released 2025-06-09) and the release notes list multiple bug fixes.- There are also reported issues across the broader
3.2.xline (e.g., CI “Channel closed”, promisify spying/mocking regressions, and hangs in certain workspace/bail/pool configurations). Keep an eye on your CI/test setup; upgrading to a newer Vitest major (v4.x) would reduce exposure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdks/nodejs/bridge_nodejs/package.json` at line 47, package.json currently pins vitest as "vitest": "^3.2.3", which is in the 3.2.x line with known CI/test instability; update package.json to either pin a stable 3.2.x patch (if you need minimal change) or upgrade to the supported major (vitest v4.x) to avoid those regressions, then run the test suite and CI to validate and address any breaking config changes (adjustments may be needed in vitest config and any test mocks/spies). Ensure you update any vitest-related scripts or config keys referenced in package.json and the repository (search for "vitest" and the test scripts) and commit the package.json and lockfile changes after verifying CI passes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@baml_language/sdk_tests/crates/typescript_node/docstrings_etc/customizable/main.test.ts`:
- Line 12: The test uses __dirname (in the readFileSync call inside docsSource()
in main.test.ts) which is undefined in ESM; replace that usage by deriving the
directory from import.meta.url (e.g. use fileURLToPath(import.meta.url) and
path.dirname or new URL('.', import.meta.url)) and update the
readFileSync(join(...)) call to use that computed directory; also add the
necessary import (fileURLToPath from 'url' or use new URL) at the top of the
test so the file read works in ESM.
---
Nitpick comments:
In `@baml_language/sdks/nodejs/bridge_nodejs/package.json`:
- Line 47: package.json currently pins vitest as "vitest": "^3.2.3", which is in
the 3.2.x line with known CI/test instability; update package.json to either pin
a stable 3.2.x patch (if you need minimal change) or upgrade to the supported
major (vitest v4.x) to avoid those regressions, then run the test suite and CI
to validate and address any breaking config changes (adjustments may be needed
in vitest config and any test mocks/spies). Ensure you update any vitest-related
scripts or config keys referenced in package.json and the repository (search for
"vitest" and the test scripts) and commit the package.json and lockfile changes
after verifying CI passes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 25460fcf-4f17-4a68-a6f9-addc493d47da
⛔ Files ignored due to path filters (13)
baml_language/sdks/nodejs/bridge_nodejs/ctx_manager.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/define_function.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/errors.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/exit_hook.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/index.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/index.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlbaml_language/sdks/nodejs/bridge_nodejs/proto.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/stream.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/stream.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/typemap.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/typemap.js.mapis excluded by!**/*.map
📒 Files selected for processing (76)
baml_language/sdk_tests/README.mdbaml_language/sdk_tests/crates/typescript_node/docstrings_etc/customizable/main.test.tsbaml_language/sdk_tests/crates/typescript_node/function_calls/customizable/methods_on_classes.test.tsbaml_language/sdk_tests/crates/typescript_node/function_calls/customizable/stdlib_entrypoints.test.tsbaml_language/sdk_tests/crates/typescript_node/llm_functions/customizable/main.test.tsbaml_language/sdk_tests/crates/typescript_node/setup.shbaml_language/sdk_tests/crates/typescript_node/src/lib.rsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/main.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_aliases.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_class_refs.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_complex_models.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_enums.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_forward_refs.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_generics.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_handles.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_lists.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_literals.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_maps.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_media.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_optional.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_primitives.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_recursion.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_routing.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_streams.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_symbol_collisions.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_unions.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_void.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/test_generic.test.tsbaml_language/sdk_tests/harness_runner/src/lib.rsbaml_language/sdk_tests/harness_setup/src/templates/package.jsonbaml_language/sdk_tests/harness_setup/src/templates/tsconfig.jsonbaml_language/sdk_tests/harness_setup/src/typescript_node.rsbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.d.tsbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.jsbaml_language/sdks/nodejs/bridge_nodejs/define_function.jsbaml_language/sdks/nodejs/bridge_nodejs/errors.jsbaml_language/sdks/nodejs/bridge_nodejs/exit_hook.jsbaml_language/sdks/nodejs/bridge_nodejs/index.d.tsbaml_language/sdks/nodejs/bridge_nodejs/index.jsbaml_language/sdks/nodejs/bridge_nodejs/native.jsbaml_language/sdks/nodejs/bridge_nodejs/package.jsonbaml_language/sdks/nodejs/bridge_nodejs/proto.jsbaml_language/sdks/nodejs/bridge_nodejs/proto/baml_cffi.d.tsbaml_language/sdks/nodejs/bridge_nodejs/proto/baml_cffi.jsbaml_language/sdks/nodejs/bridge_nodejs/stream.d.tsbaml_language/sdks/nodejs/bridge_nodejs/stream.jsbaml_language/sdks/nodejs/bridge_nodejs/tests/call_function.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/host_callable.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/run_vitest.rsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_collector.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_decode_call_result.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_decode_handle.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_engine.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_media.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_stream.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_tracing.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_typemap.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tsconfig.jsonbaml_language/sdks/nodejs/bridge_nodejs/typemap.jsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/copy-native-dts.jsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/copy-proto.jsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/ctx_manager.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/define_function.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/exit_hook.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/index.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.jsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/stream.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/tag-generated-files.jsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/typemap.tsbaml_language/sdks/nodejs/bridge_nodejs/vitest.config.tsbaml_language/sdks/nodejs/codegen_nodejs/src/emit/typemap_file.rsbaml_language/sdks/nodejs/codegen_nodejs/src/leaf.rsbaml_language/sdks/nodejs/codegen_nodejs/src/lib.rsbaml_language/sdks/nodejs/codegen_nodejs/src/routing.rs
✅ Files skipped from review due to trivial changes (26)
- baml_language/sdks/nodejs/bridge_nodejs/tests/test_engine.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/tests/test_decode_call_result.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/tests/test_collector.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts
- baml_language/sdks/nodejs/bridge_nodejs/ctx_manager.d.ts
- baml_language/sdks/nodejs/bridge_nodejs/proto/baml_cffi.d.ts
- baml_language/sdks/nodejs/bridge_nodejs/stream.d.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_handles.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/tests/call_function.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/stream.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.d.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_symbol_collisions.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/ctx_manager.ts
- baml_language/sdks/nodejs/bridge_nodejs/tests/test_decode_handle.test.ts
- baml_language/sdk_tests/crates/typescript_node/llm_functions/customizable/main.test.ts
- baml_language/sdk_tests/crates/typescript_node/setup.sh
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_streams.test.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_unions.test.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_void.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/stream.js
- baml_language/sdk_tests/README.md
- baml_language/sdks/nodejs/bridge_nodejs/index.d.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_literals.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/ctx_manager.js
- baml_language/sdk_tests/crates/typescript_node/src/lib.rs
- baml_language/sdks/nodejs/bridge_nodejs/proto.js
🚧 Files skipped from review as they are similar to previous changes (38)
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/typemap.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/exit_hook.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/index.ts
- baml_language/sdk_tests/crates/typescript_node/function_calls/customizable/methods_on_classes.test.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_lists.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/tests/run_vitest.rs
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_complex_models.test.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_routing.test.ts
- baml_language/sdk_tests/harness_setup/src/templates/package.json
- baml_language/sdks/nodejs/bridge_nodejs/tests/test_tracing.test.ts
- baml_language/sdk_tests/crates/typescript_node/function_calls/customizable/stdlib_entrypoints.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/tsconfig.json
- baml_language/sdks/nodejs/bridge_nodejs/vitest.config.ts
- baml_language/sdks/nodejs/bridge_nodejs/tests/test_media.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/tests/host_callable.test.ts
- baml_language/sdks/nodejs/codegen_nodejs/src/routing.rs
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/copy-proto.js
- baml_language/sdks/nodejs/bridge_nodejs/tests/test_typemap.test.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_optional.test.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_generics.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/tests/test_stream.test.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_class_refs.test.ts
- baml_language/sdk_tests/harness_setup/src/templates/tsconfig.json
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_primitives.test.ts
- baml_language/sdk_tests/harness_runner/src/lib.rs
- baml_language/sdks/nodejs/codegen_nodejs/src/emit/typemap_file.rs
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_forward_refs.test.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_aliases.test.ts
- baml_language/sdks/nodejs/codegen_nodejs/src/lib.rs
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_recursion.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/native.js
- baml_language/sdks/nodejs/bridge_nodejs/define_function.js
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/main.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/index.js
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/tag-generated-files.js
- baml_language/sdk_tests/harness_setup/src/typescript_node.rs
- baml_language/sdks/nodejs/bridge_nodejs/exit_hook.js
- baml_language/sdks/nodejs/codegen_nodejs/src/leaf.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/sdks/nodejs/bridge_nodejs/package.json`:
- Around line 38-39: The test script is not self-sufficient because many tests
import built artifacts from ../dist (e.g., tests/test_tracing.test.ts and
tests/test_decode_call_result.test.ts), so update the package.json "test" script
to run the build step before running Vitest (invoke the existing build:debug
script or equivalent) so pnpm test builds the project and then runs "vitest
run"; modify the "test" script entry accordingly to chain the build (ensuring it
fails fast on build errors) and keep the existing "attw" and other scripts
untouched.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5be2d1f3-f251-4ca8-ad87-a7e4deca3ed9
⛔ Files ignored due to path filters (54)
baml_language/sdks/nodejs/bridge_nodejs/ctx_manager.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/ctx_manager.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/define_function.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/define_function.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/ctx_manager.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/ctx_manager.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/ctx_manager.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/ctx_manager.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/define_function.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/define_function.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/define_function.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/define_function.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/errors.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/errors.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/errors.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/errors.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/exit_hook.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/exit_hook.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/exit_hook.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/exit_hook.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/index.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/index.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/index.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/index.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/native.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/native.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/proto.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/proto.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/proto.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/proto.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/proto/baml_cffi.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/proto/baml_cffi.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/stream.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/stream.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/stream.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/stream.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/typemap.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/typemap.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/typemap.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/typemap.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/errors.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/errors.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/exit_hook.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/exit_hook.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/index.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/index.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlbaml_language/sdks/nodejs/bridge_nodejs/proto.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/proto.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/stream.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/stream.js.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/typemap.d.ts.mapis excluded by!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/typemap.js.mapis excluded by!**/*.mappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (68)
baml_language/Cargo.tomlbaml_language/sdk_tests/README.mdbaml_language/sdk_tests/crates/typescript_node/docstrings_etc/customizable/main.test.tsbaml_language/sdk_tests/crates/typescript_node/function_calls/customizable/methods_on_classes.test.tsbaml_language/sdk_tests/crates/typescript_node/function_calls/customizable/stdlib_entrypoints.test.tsbaml_language/sdk_tests/crates/typescript_node/llm_functions/customizable/main.test.tsbaml_language/sdk_tests/crates/typescript_node/setup.ps1baml_language/sdk_tests/crates/typescript_node/setup.shbaml_language/sdk_tests/crates/typescript_node/src/lib.rsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/main.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_aliases.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_class_refs.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_complex_models.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_enums.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_forward_refs.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_generics.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_handles.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_lists.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_literals.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_maps.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_media.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_optional.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_primitives.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_recursion.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_routing.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_streams.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_symbol_collisions.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_unions.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_void.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/test_generic.test.tsbaml_language/sdk_tests/harness_runner/src/lib.rsbaml_language/sdk_tests/harness_setup/src/templates/package.jsonbaml_language/sdk_tests/harness_setup/src/templates/tsconfig.jsonbaml_language/sdk_tests/harness_setup/src/typescript_node.rsbaml_language/sdks/nodejs/bridge_nodejs/.gitignorebaml_language/sdks/nodejs/bridge_nodejs/index.jsbaml_language/sdks/nodejs/bridge_nodejs/native.jsbaml_language/sdks/nodejs/bridge_nodejs/package.jsonbaml_language/sdks/nodejs/bridge_nodejs/tests/call_function.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/host_callable.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/run_vitest.rsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_collector.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_decode_call_result.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_decode_handle.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_engine.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_media.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_stream.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_tracing.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_typemap.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tsconfig.jsonbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/clean-dist.jsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/copy-native-dts.jsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/copy-proto.jsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/ctx_manager.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/define_function.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/exit_hook.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/index.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.jsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/stream.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/tag-generated-files.jsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/typemap.tsbaml_language/sdks/nodejs/bridge_nodejs/vitest.config.tsbaml_language/sdks/nodejs/codegen_nodejs/src/emit/typemap_file.rsbaml_language/sdks/nodejs/codegen_nodejs/src/leaf.rsbaml_language/sdks/nodejs/codegen_nodejs/src/lib.rsbaml_language/sdks/nodejs/codegen_nodejs/src/routing.rs
💤 Files with no reviewable changes (2)
- baml_language/sdks/nodejs/bridge_nodejs/native.js
- baml_language/sdks/nodejs/bridge_nodejs/index.js
✅ Files skipped from review due to trivial changes (16)
- baml_language/sdks/nodejs/bridge_nodejs/.gitignore
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_handles.test.ts
- baml_language/sdk_tests/crates/typescript_node/src/lib.rs
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_class_refs.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/tests/test_decode_handle.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/tests/call_function.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/stream.ts
- baml_language/sdks/nodejs/bridge_nodejs/tests/test_collector.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/tests/test_decode_call_result.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/tests/run_vitest.rs
- baml_language/sdks/nodejs/codegen_nodejs/src/routing.rs
- baml_language/sdk_tests/crates/typescript_node/docstrings_etc/customizable/main.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/tests/test_typemap.test.ts
- baml_language/sdks/nodejs/codegen_nodejs/src/lib.rs
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/exit_hook.ts
🚧 Files skipped from review as they are similar to previous changes (35)
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_generics.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/tests/test_stream.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/tests/test_engine.test.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_maps.test.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_streams.test.ts
- baml_language/sdk_tests/crates/typescript_node/function_calls/customizable/stdlib_entrypoints.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/tests/host_callable.test.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_optional.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.d.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/main.test.ts
- baml_language/sdk_tests/crates/typescript_node/llm_functions/customizable/main.test.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_primitives.test.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_unions.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/define_function.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_literals.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/tests/test_media.test.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_complex_models.test.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_recursion.test.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_forward_refs.test.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_symbol_collisions.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/vitest.config.ts
- baml_language/sdk_tests/harness_setup/src/templates/tsconfig.json
- baml_language/sdk_tests/README.md
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_aliases.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/copy-native-dts.js
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_media.test.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_enums.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/typemap.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/tag-generated-files.js
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/ctx_manager.ts
- baml_language/sdks/nodejs/codegen_nodejs/src/emit/typemap_file.rs
- baml_language/sdk_tests/harness_setup/src/templates/package.json
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_lists.test.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/test_generic.test.ts
- baml_language/sdks/nodejs/codegen_nodejs/src/leaf.rs
| "attw": "attw --pack", | ||
| "test": "vitest run" |
There was a problem hiding this comment.
Make pnpm test self-sufficient again.
The suites now import built artifacts from ../dist/... (for example, tests/test_tracing.test.ts Line 3 and tests/test_decode_call_result.test.ts Lines 8-9), but the package script only runs vitest. On a clean checkout, pnpm test will fail unless build:debug was run manually first; tests/run_vitest.rs currently hides that by building before it calls the script.
Suggested fix
+ "pretest": "pnpm build:debug",
- "test": "vitest run"
+ "test": "vitest run"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baml_language/sdks/nodejs/bridge_nodejs/package.json` around lines 38 - 39,
The test script is not self-sufficient because many tests import built artifacts
from ../dist (e.g., tests/test_tracing.test.ts and
tests/test_decode_call_result.test.ts), so update the package.json "test" script
to run the build step before running Vitest (invoke the existing build:debug
script or equivalent) so pnpm test builds the project and then runs "vitest
run"; modify the "test" script entry accordingly to chain the build (ensuring it
fails fast on build errors) and keep the existing "attw" and other scripts
untouched.
- Run cargo fmt --all -- --config imports_granularity=Crate --config group_imports=StdExternalCrate - Add --profile esm-only flag to attw test to ignore CJS resolution failures - Update package.json attw script to match Co-authored-by: Sam Lijin <[email protected]>
Implement best practices for nodejs publish/sdkgen.
This note evaluates the current
sam/node-esmimplementation against thebest-practice sources listed in the prompt:
EcmaScript imports
exportsandtypesconditionsShort answer: yes on the core ESM package recommendations, and now mostly
yes on publish-shape validation too.
The branch now implements the high-value recommendations:
@boundaryml/baml-core-nodeis ESM-only."type": "module".exports, with no top-levelmain."types"condition first.dist/.module: "nodenext"andmoduleResolution: "nodenext"..jsrelativeimport specifiers.
esm_outputregression check.attw --packagainst the bridge package.@napi-rs/cli3.7.0napi3.9.0napi-derive3.5.6napi-build2.3.2Remaining gaps are smaller and more policy-dependent:
engines.nodefield yetsideEffectsfield yetprepublishOnlypipeline yetpublintcheck yetCurrent Package Shape
The bridge package now has the package metadata shape recommended by the
ESM-first sources:
{ "name": "@boundaryml/baml-core-node", "type": "module", "types": "./dist/index.d.ts", "exports": { ".": { "types": "./dist/index.d.ts", "import": "./dist/index.js" } }, "files": ["dist"] }This is the important change from the earlier version of this note. We no
longer publish root-level
index.js,native.js,proto.js, or root.d.tsfiles. The package boundary points into
dist/, andnpm pack --dry-runshows the tarball as
package.jsonplusdist/**.The package is exports-only. There is no top-level
main, which matchesSindre's pure-ESM recommendation to replace
mainwithexports.What We Now Conform To
1. ESM-only is a defensible choice for a new package
Anthony Fu's newer guidance is explicit: new packages should usually ship
ESM-only, especially when they target evergreen Node or modern toolchains.
Sindre Sorhus takes the same strong position for pure ESM packages.
sam/node-esmfollows that. The package has only animportcondition:{ "exports": { ".": { "types": "./dist/index.d.ts", "import": "./dist/index.js" } } }There is no
requirecondition. That is intentional. CommonJS consumers mustuse dynamic
import(...):That is the breaking change we chose. It conforms to the ESM-only guidance,
not to the older dual-package compatibility strategy.
2. The emitted entrypoint is really ESM
The invalid pattern is:
"type": "module".index.jsas ESM.exports,module.exports, orrequire.The current implementation avoids that.
bridge_nodejs/tsconfig.jsonuses:{ "compilerOptions": { "target": "es2022", "module": "nodenext", "moduleResolution": "nodenext", "rootDir": "./typescript_src", "outDir": "./dist" } }The generated
dist/index.jshas real ESM syntax:That conforms to Matt Pocock's package guide, Sindre's pure-ESM checklist, and
the TypeScript 4.7 Node ESM model.
3. Relative imports use
.jsMatt Pocock's NodeNext import-extension article and Sindre's pure-ESM guide
both call out the same rule: in TypeScript source compiled under Node-style
ESM, relative imports should point at the emitted JavaScript file name:
not:
The branch follows this in two places:
bridge_nodejs/typescript_srccodegen_nodejsThe SDK tests enforce it. Each TypeScript fixture gets an
esm_outputtestthat scans
generated/baml_sdk/**/*.tsand fails if a relativefrom "./..."import omits
.js.4. The
exportsmap is intentionally import-onlyNode's package docs define conditional exports as the mechanism for routing
different entrypoints for
importandrequire. Anthony Fu's dual-packagearticle shows the dual form:
{ "exports": { ".": { "types": "./dist/index.d.ts", "import": "./dist/index.mjs", "require": "./dist/index.cjs" } } }We intentionally do not do that. Our map is:
{ "exports": { ".": { "types": "./dist/index.d.ts", "import": "./dist/index.js" } } }That is ESM-only, not dual ESM/CJS. TypeScript's
"types"condition comesfirst, matching the TypeScript 4.7 guidance that
"types"should be first inconditional exports.
One nuance: some checklists use
"default"instead of or in addition to"import". Our map has only"import". That is fine for Node ESM andTypeScript's NodeNext resolver. A
"default"condition could improveunknown-runtime compatibility, but it is not required for this Node-targeted
package.
5. We now publish from
dist/Matt Pocock's package guide uses:
{ "files": ["dist"], "main": "dist/index.js" }In our ESM-only package we use the same
dist/publishing idea, but throughexportsinstead ofmain:{ "types": "./dist/index.d.ts", "exports": { ".": { "types": "./dist/index.d.ts", "import": "./dist/index.js" } }, "files": ["dist"] }This is now aligned with the package-layout recommendation. The build emits:
dist/index.jsdist/index.d.tsdist/native.jsdist/native.d.tsdist/baml_node.<platform>.nodedist/proto.jsdist/proto/baml_cffi.jsThe napi wrapper is generated directly at
dist/native.js. The platform.nodefile is copied intodist/sodist/native.jscan load a local./baml_node.<platform>.nodefile from the packed package.6. Internal
createRequireindist/native.jsis acceptableSindre's guide says to replace
require()andmodule.exportswithimport/export. That rule is about the module's public syntax and ordinarydependency loading.
The napi-generated native wrapper is different. It is an ESM file:
It uses
createRequireinternally to load.nodenative addon files andplatform-specific native packages. That is a normal ESM escape hatch for Node
APIs and native addons. The important package boundary remains ESM:
module.exportsdist/index.jsSo this conforms in spirit. It is not the bad pattern of publishing a CJS file
as
.jsunder"type": "module".7. Protobuf output is ESM-compatible
The build uses:
and post-processes the generated import to:
The generated protobuf runtime is copied to
dist/proto/baml_cffi.js, whichmatches the runtime import from
dist/proto.js.8. Tests use Vitest
Matt Pocock's package guide uses Vitest as the test runner. We now use Vitest
in both:
bridge_nodejs/package.jsonsdk_test_typescript_nodefixture packagesThe bridge package tests import from
../dist/..., so they exercise the samelayout the package publishes.
The generated fixture package template has:
{ "type": "module", "scripts": { "test": "vitest run" } }That is aligned with the new-package guidance and avoids Jest's historically
awkward ESM behavior.
9. Are The Types Wrong is now part of sdk_test_typescript_node
Mark Erikson's ESM modernization writeup is the strongest warning that package
exports, ESM/CJS conditions, and TypeScript declarations are easy to get
subtly wrong. Are The Types Wrong exists specifically to validate the packed
artifact shape across TypeScript module-resolution modes.
The bridge package now has:
{ "scripts": { "attw": "attw --pack" }, "devDependencies": { "@arethetypeswrong/cli": "0.18.3" } }The
sdk_test_typescript_nodescaffold emits a package-level test:This is better than only testing repo-local source files. It checks the
package the way consumers install it: by packing the npm artifact and
analyzing its
exportsand declarations.10. Napi is on current versions
The current Node and Rust napi toolchain versions are up to date for this
branch:
{ "devDependencies": { "@napi-rs/cli": "3.7.0" } }This is not directly an ESM best-practice item, but it matters here because
native ESM support depends on the napi-rs generated wrapper.
What Still Does Not Fully Conform
1. No
engines.nodefield yetSindre recommends a modern Node floor, such as:
{ "engines": { "node": ">=18" } }We do not currently declare an engine. Given the current toolchain:
@arethetypeswrong/clirequires Node>=20I would probably add:
{ "engines": { "node": ">=20" } }That is a policy decision, because it changes the package's declared runtime
support floor. If BAML wants to support Node 18 consumers, then
attwbeing adev dependency with Node
>=20is not by itself a reason to make the runtimepackage require Node 20. But the support floor should be explicit.
2. No
sideEffectsfield yetThe distilled checklist includes:
{ "sideEffects": false }I would not add that blindly here.
This package initializes native bindings, exposes runtime singletons, installs
exit hooks, and generated SDK entrypoints call runtime initialization at module
evaluation time. That does not automatically mean every export is
side-effectful, but it does mean
sideEffects: falseneeds an audit.For this package, omitting
sideEffectsis safer than claiming it istree-shake-pure.
3. No
prepublishOnlypipeline yetMatt Pocock's guide recommends a publish-time validation pipeline. We now have
the ingredients:
attw --packBut the package does not yet have:
{ "scripts": { "prepublishOnly": "pnpm build:release && pnpm test && pnpm attw" } }The exact command should account for this monorepo's release flow and native
matrix build. Still, some release-time gate should run build, tests, and ATTW
against the packed artifact.
4. No
publintcheck yetATTW focuses on TypeScript package entrypoint/type correctness.
publintchecks a different slice of package-publishing problems. Given Mark Erikson's
warning about how easy ESM package metadata is to get subtly wrong, adding
publintwould be a reasonable next hardening step.5. We do not ship dual ESM/CJS
This is not a non-conformance with the new-package guidance. It is a
deliberate non-conformance with the older dual-package compatibility strategy.
Anthony Fu's dual-package article is useful if we decide CommonJS users still
matter enough to support:
{ "exports": { ".": { "types": "./dist/index.d.ts", "import": "./dist/index.mjs", "require": "./dist/index.cjs" } } }But the newer guidance says new packages should generally move on to ESM-only.
Our stated product decision was to break CommonJS users and require dynamic
import(...). So the lack of arequirecondition is correct for this branch.Suggested Follow-Ups
If we want
@boundaryml/baml-core-nodeto be closer to the full 2026best-practice checklist, I would do these in order:
>=20if that matches thesupport policy.
prepublishOnlyor equivalent CI packaging gate.publint.sideEffects: false."default": "./dist/index.js"condition if we wantbetter unknown-runtime compatibility beyond Node's
"import"condition.Verdict
sam/node-esmnow conforms to the core ESM and package-shape best practices:typescondition firstmaindist/publishing layout.jsrelative importssdk_test_typescript_nodeThe remaining work is release-policy hardening, not the ESM conversion itself:
engines.nodesideEffectspublintSummary by CodeRabbit
New Features
Refactor
distdirectory for cleaner distribution..jsfile extensions for ESM compatibility.