Fix: gate the esbuild createRequire preamble on the Node platform - #555
Open
AmaadMartin wants to merge 2 commits into
Open
Fix: gate the esbuild createRequire preamble on the Node platform#555AmaadMartin wants to merge 2 commits into
AmaadMartin wants to merge 2 commits into
Conversation
added 2 commits
August 2, 2026 18:55
The createRequire preamble exists so Node ESM output can reach CommonJS-only dependencies, but it was applied to every ESM target, including the browser build. That left an unresolvable bare 'module' import in dist/web, the artifact the package browser field points at. Gate the preamble on platform === 'node' as well as format === 'esm'. The Node esm/cjs artifacts are byte-identical before and after.
…uiltin Reads the already-built dist trees for core and integrations and fails if any emitted browser file imports the Node 'module' builtin. A companion assertion pins core/dist/esm/index.js to keep the preamble so the browser fix cannot be re-implemented by deleting it everywhere. The hooks fail with an actionable message when dist/web is absent or empty, so a missing build cannot make the suite pass vacuously.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
Link to an existing issue (if applicable):
N/A
Or, if no issue exists, describe the change:
Problem:
@google/adkand@google/adk-integrationsboth advertise a browser build ("browser": "./dist/web/index_web.js"), but that artifact cannot be loaded by a browser or consumed by a browser-targeted bundler without extra configuration.core/build.jsandintegrations/build.jsappend a CommonJS-interop preamble whenever the output format is ESM:main()builds three targets:esm(node/esm),cjs(node/cjs) andweb(browser/esm). Because the gate isformat === 'esm'alone, the browser target gets the preamble too, andpackages: 'external'leaves the bare'module'specifier untouched in the emitted JavaScript. Measured on this branch before the fix: all 198 files undercore/dist/web/and all 3 underintegrations/dist/web/— including bothindex_web.jsfiles thebrowserfields point at — import the Nodemodulebuiltin.Solution: Add the platform to the gate — one line per build script, plus a comment explaining why the preamble exists at all:
The preamble is only needed so Node ESM output can reach CommonJS-only dependencies (esbuild lowers
await import(pkg)of an external package torequire(pkg)for thenode10.4target). It has no purpose in a browser bundle. The body of the block is left byte-for-byte unchanged, deliberately: a separate queued change rewrites the specifier to'node:module'inside the same block, and touching only the condition keeps the two changes conflict-free (the new test's regex accepts either specifier, so it passes in both orderings).Resulting banner per target:
esm(node, non-bundle)esm(node, bundle)cjs(node)web(browser, non-bundle)web(browser, bundle)Deliberately not done here (kept the diff minimal and reviewable): the two build scripts are near-duplicates by design, and this change does not extract a shared helper, reformat surrounding code, or refactor either script.
Known limitation, called out deliberately: exactly one emitted file,
core/dist/web/sessions/db/operations.js, actually callsrequire(...)— esbuild lowersawait import('@mikro-orm/…')to__toESM(require('@mikro-orm/…'))for thechrome58target — so it loses a definedrequire. That module is not reachable from the browser entry (bundlingsrc/index_web.tsproduces no@mikro-ormreference; the onlyrequire(occurrences in the bundled web output are inside string literals fromsrc/tools/skill/run_skill_script_tool.ts). It is emitted intodist/webonly because non-bundle mode transpiles all ofsrc/**/*.ts, and it depends on Node-only ORM packages, so it could never run in a browser either way. Trading an unresolvable bare import for an undefinedrequirein an already-unusable module is a net improvement.Out of scope:
dist/webalso importsnode:path,node:fs/promises,node:netandnode:dns/promisesthroughsrc/common.ts's export graph. Onlynode:async_hooksis shimmed today. The new test is therefore scoped to themodulebuiltin — the one this fix removes — and does not assert "no Node builtins at all", which cannot pass today. The broader browser-compatibility cleanup is tracked separately.Collision check (fork
AmaadMartin/adk-js, all 379 open PRs scanned): three open PRs touch the same banner block, none of them makes this change, so this is not a duplicate:'module'->'node:module'inside the block. Orthogonal; this PR does not touch that string.format === 'esm'-only, so the browser output keeps the preamble under that PR.This PR is branched from
mainrather than stacked on any of them: none is a prerequisite, the edits are semantically independent (condition vs. banner contents vs. banner ordering), and stacking on one would still leave the other two overlapping while suppressing CI on a non-mainbase.Testing Plan
Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.
The changed lines live in build scripts, which are outside the vitest coverage
includeglobs (core/src/**,dev/src/**,integrations/src/**), so they cannot be covered by a unit test and cannot move the configured thresholds — no threshold was edited. Both branches of the changed condition are covered behaviourally by one new integration test,tests/integration/build_output/web_output_test.ts(picked up by the existingtests/integration/**/*_test.tsglob; novitest.config.tschange):coreandintegrations: every emitted.jsfile underdist/webis read and asserted not to match/from\s*['"](?:node:)?module['"]/, collecting offending paths into an array so a failure names the files;dist/web/index_web.js(the file eachpackage.jsonbrowserfield resolves to) is asserted to exist;describe.each,core/dist/esm/index.jsmust match the same regex, so the browser build cannot be "fixed" later by deleting the preamble everywhere.The test reads the already-built
disttrees rather than running a build, because other integration tests consumecore/distviafile:workspace fixtures and rewritingdistfrom inside a test would race them. CI builds before it tests (validation.yaml: "Build packages" precedes "Run tests and check code coverage").Failure paths exercised (a missing build must not produce a vacuous pass):
integrations/dist/webremovedAssertionError: …/integrations/dist/web is missing. Run \npm run build` first.`integrations/dist/webpresent but emptyAssertionError: …/integrations/dist/web holds no .js files. Run \npm run build` first.`Proof the tests can fail (each mutation applied,
npm run buildre-run, test re-run):core/build.jsgate reverted toif (format === 'esm') {core browser build > never imports the Node module builtin—AssertionError: expected [ 'common.js', 'index.js', …(196) ] to deeply equal [], the diff naming"index_web.js"integrations/build.jsgate reverted toif (format === 'esm') {integrations browser build > never imports the Node module builtin—AssertionError: expected [ 'index.js', 'index_web.js', …(1) ] to deeply equal []if (…) { buildOptions.banner = … }block deleted fromcore/build.jscore Node ESM build > keeps the createRequire preamble—AssertionError: expected '/**\n * @license\n * Copyright 2025 G…' to match /from\s*['"](?:node:)?module['"]/Node artifacts are provably untouched.
sha256sumover every file incore/dist/{esm,cjs}+integrations/dist/{esm,cjs}, built with the fix, with the fix reverted, and with the fix restored, is the same digest all three times (a04a2678…), confirming the fix was not implemented by deleting the preamble.Manual End-to-End (E2E) Tests:
All observed as expected with esbuild 0.25.12 (resolved from the root
devDependenciesrange^0.25.9) on Node v22.22.2.Local validation on the exact pushed commit:
npm run build✓,npx vitest run --project integration tests/integration/build_output/web_output_test.ts✓ (5/5),npm run lint✓ (exit 0),npx prettier --checkon all three changed files ✓. No new@ts-expect-error/@ts-ignore/eslint-disable/anyanywhere in the diff.Checklist
[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.