fix(build): only shim node:url/path/module in ESM files that need them#2365
Merged
Conversation
The tsup ESM banner (node:url/node:path/node:module + __dirname/__filename/ require shims) was applied unconditionally via esbuildOptions to every .mjs output, including pure-data files like enums.generated.mjs that never touch those globals. Strict browser bundlers (Vite, esbuild --platform=browser) can't resolve real Node built-ins, so importing @adcp/sdk/enums or other lean entry points broke bundling for browser consumers. Replace the blanket banner with an esbuild onLoad plugin that only prepends the shim to files whose transpiled body actually references __dirname/__filename/require (11 of 486 files) — detected via a throwaway transformSync so comments merely mentioning "require()" don't false-positive. Runs pre-transform so sourcemaps stay accurate. Add a browser-platform esbuild bundle check to verify-package.mjs so this class of regression fails CI going forward. Fixes #2364
Two follow-ups from review: - require(...features) in SingleAgentClient.ts is a class method, not a call to the global require — the detection regex matched it anyway, giving that file the shim unnecessarily. Every real require() call in the codebase takes a string-literal module specifier, so require the match to have a following quote. - __filename is never read by any source file; it only existed as an intermediate step toward __dirname. Compute __dirname inline instead of binding an unused name.
benminer
approved these changes
Jul 17, 2026
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
2 participants
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.
Summary
node:url/node:path/node:module+__dirname/__filename/requireshims) was applied unconditionally viaesbuildOptionsto every.mjsoutput — including pure-data files likeenums.generated.mjsthat never touch those globals.esbuild --platform=browser) can't resolve real Node built-ins, so importing@adcp/sdk/enumsor other lean entry points broke bundling for browser consumers.onLoadplugin that only prepends the shim to files whose transpiled body actually references__dirname/__filename/require— measured at 11 of 486.mjsfiles. Detection uses a throwawaytransformSync(comments/types stripped) rather than raw source matching, so a comment merely mentioningrequire()can't cause a false positive. Runs pre-transform so esbuild's own sourcemap generation stays accurate for the shimmed files.--platform=browseresbuild bundle check toscripts/verify-package.mjs(already wired into CI fortsup.config.tschanges) so this exact regression fails CI if it recurs.Fixes #2364.
Test plan
dist/lib/types/enums.generated.mjswithesbuild --bundle --platform=browserbefore the fix..mjsfiles retain the shim, matching the files that reference__dirname/__filename/requirein their body.node scripts/verify-package.mjspasses (ESM/CJS load + new browser-bundle check).npm run check:package(publint + attw) — all green.npm run typecheck— clean.npm run test:node:fast— same 2 pre-existing failures as onmain(unrelated JWK/crypto signing tests), confirmed via bisecting withgit stash; no new failures introduced.🤖 Generated with Claude Code