fix(build): inject createRequire banner to support CJS deps in ESM bundles#166
Conversation
…ndles Assisted-By: docker-agent
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The createRequire banner is the standard, idiomatic fix for this class of ESM/CJS interop problem and the implementation is correct. The banner syntax is valid, node:module has been stable since Node.js 12, and the fix is well-explained in the accompanying comments.
Two minor fragility observations are noted below (no action required for merge).
| // as a top-level banner supplies a real require() before the shim runs, | ||
| // so those CJS modules can load Node.js built-ins normally. | ||
| banner: { | ||
| js: "import { createRequire } from 'node:module'; const require = createRequire(import.meta.url);", |
There was a problem hiding this comment.
[LOW] const require — consider var for resilience against future collisions
The banner declares const require, which is block-scoped and non-redeclarable. Under the current esbuild version this is safe — esbuild's own CJS shim uses var __require (a different identifier). However, if a future esbuild upgrade changes the helper name to require, or if another plugin also injects a require binding, the module will throw SyntaxError: Identifier 'require' has already been declared at startup rather than silently overriding.
Using var require instead would degrade gracefully in that scenario (last-write wins) and matches the precedent set by tools like Vite and Rollup when they inject the same shim.
This is not a current bug — just a minor resilience note.
There was a problem hiding this comment.
Done — switched to var in commit 843c614. Using var avoids any risk of a SyntaxError if esbuild ever emits a hoisted var require of its own in the same scope (where const would be a re-declaration), and it also avoids TDZ issues if the shim is somehow evaluated before the banner binding is initialised.
| // throws "Dynamic require of 'net' is not supported". Injecting createRequire | ||
| // as a top-level banner supplies a real require() before the shim runs, | ||
| // so those CJS modules can load Node.js built-ins normally. | ||
| banner: { |
There was a problem hiding this comment.
[LOW] Banner uses ESM-only syntax — will silently break if 'cjs' is ever added to format
The injected banner contains an import statement and import.meta.url, both of which are syntax errors in CommonJS output. The current config only emits ESM (format: ['esm']), so this is harmless today.
If a future maintainer extends format to ['esm', 'cjs'] (a common tsup pattern), the banner will be emitted verbatim into .cjs files and cause a parse failure. A short comment on the banner block noting the ESM-only constraint would guard against this:
// NOTE: this banner uses import/import.meta.url — only valid in ESM.
// If format is ever extended to include 'cjs', this must be conditioned
// or moved to a format-specific banner (banner: { esm: '...' }).
banner: {
js: "import { createRequire } from 'node:module'; const require = createRequire(import.meta.url);",
},Not a current bug; just a maintenance note.
There was a problem hiding this comment.
Done — the three-line ESM-only warning comment has been added directly above the banner: block in commit 843c614. It calls out explicitly that the import/import.meta.url syntax is only valid in ESM output and what action to take if 'cjs' is ever added to format.
Assisted-By: docker-agent
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
Fix is correct, well-reasoned, and empirically verified.
The createRequire banner pattern is the standard solution for this class of CJS-in-ESM bundling problem and has been verified to work correctly with tsup 8.5.1 / esbuild for this project.
Mechanism (verified empirically):
- Banner injects
const require = createRequire(import.meta.url)at line 1 of each bundle - esbuild's
__requireshim (line 8) immediately evaluatestypeof require !== "undefined"→true→ returns the banner'srequireby reference __requirebecomes identical to thecreateRequire-backed functiontunnel's__commonJSwrapper calls__require("net")→ works correctlyglobalThis.requirestaysundefined— no global scope leakage
Verified with actual build output:
dist/credentials.js(1.84 MB) starts with the banner on line 1 ✅dist/signed-commit.js(148 KB) gets the banner too, harmlessly ✅node dist/credentials.jsgets past thetunnelinit crash ✅biome ci .andtsc --noEmitpass ✅
One inline note on a minor security nuance (see below) — not a blocker.
| // as a top-level banner supplies a real require() before the shim runs, | ||
| // so those CJS modules can load Node.js built-ins normally. | ||
| // NOTE: this banner uses import/import.meta.url — only valid in ESM output. | ||
| // If format is ever extended to include 'cjs', this must be conditioned or |
There was a problem hiding this comment.
[LOW] Module-scoped require in security-sensitive bundle — acknowledged
const require = createRequire(import.meta.url) introduces a working require() into the module scope of every output bundle. In pure ESM without this banner, no require exists at all.
A few things are worth being aware of (not blockers):
- The
requireis module-scoped, not global —globalThis.requirestaysundefined, so only code already executing inside the bundle can call it. - All CJS
require()calls in bundled deps route through esbuild's__requireshim (which delegates to thisrequire), so the exposure is bounded to the bundle's existing dependency graph. - None of the current dependencies (
tunnel,@actions/core, AWS SDK) are known to pass user-controlled input torequire()— but the action handles API keys and signed commits, so it's worth keeping in mind if new CJS dependencies are added.
Empirically confirmed to not cause issues in any current code path. ✅
Problem
The
releaseworkflow was failing at the "Setup credentials" step with:Root cause
Commit
83bd0cdswitched the bundler from Vite → tsup, emitting ESM output (.jswith"type": "module") and bundling all npm dependencies vianoExternal: [/.*/].The dependency chain that causes the issue:
tunnel@0.0.6is a CommonJS package. When esbuild bundles it into an ESM file it wraps it in a__commonJSclosure and generates a top-level__requireshim:In ESM mode there is no global
require, so the shim falls through tothrowevery timetunnelcallsrequire('net'). Settingplatform: 'node'in tsup does mark Node.js built-ins as external for top-level ESM imports, but it does not inject a CJS→ESM bridge for built-ins referenced inside__commonJSwrappers.Fix
Add a
bannertotsup.config.tsthat injectscreateRequireat the very top of every output file:When
__requireevaluatestypeof require !== "undefined"it now finds thecreateRequire-backed function and delegates to it, allowing CJS modules to successfully load any Node.js built-in (net,tls,http,https, …).Verification
biome ci .— ✅ cleantsc --noEmit— ✅ cleanFixes the failing run: https://github.com/docker/cagent-action/actions/runs/25336725173/job/74283730978