fix(build): swap Vite for tsup to fix DOMParser ReferenceError#163
Conversation
The Vite-bundled output of `dist/credentials.js` was pulling in the browser-targeted Smithy XML codec from the AWS SDK, which references `DOMParser` — undefined in Node.js. The action runs as a composite GHA step (`node dist/credentials.js`) so the build must produce Node-targeted output. Swap Vite → tsup with `platform: 'node'` and `target: 'node24'` so esbuild resolves the AWS SDK's `node` export condition. Bundle every npm dep (`noExternal: [/.*/]`) since the action runs without node_modules. Also bump @aws-sdk/client-secrets-manager 3.787.0 → 3.972.0 to match the @aws-sdk/credential-provider-web-identity 3.972.x family already in use, so all SDK packages share one Smithy runtime. Verified: dist builds cleanly, 0 DOMParser refs in output, 30/30 unit tests pass, typecheck + biome ci pass. Assisted-By: docker-agent
|
cc @derekmisler — ready for review when you are |
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The Vite → tsup migration correctly resolves the DOMParser is not defined ReferenceError. Setting platform: 'node' forces esbuild to resolve the Node.js export conditions in AWS SDK packages (preventing the browser DOMParser-using Smithy XML codec from being bundled), and noExternal: [/.*/] correctly ensures all npm dependencies are self-contained in the output for a GHA environment without node_modules.
Key verifications performed:
noExternal: [/.*/]+platform: 'node'is safe: esbuild unconditionally keeps all Node.js built-ins (includingnode:-prefixed forms) external at the esbuild core layer —noExternalonly affects tsup'sexternalizeDepsPluginlayer and cannot override esbuild's built-in exclusion. ✅- Vitest config extraction to
vitest.config.tsis complete and functionally equivalent. ✅ tsconfig.jsonexclude list correctly swappedvite.config.tsfortsup.config.tsandvitest.config.ts. ✅@aws-sdk/client-secrets-managerbump to3.972.0is consistent with the lockfile. ✅
Minor notes (no action required):
splitting: falsewith two entry points means each output file is fully self-contained, which the PR description acknowledges. The AWS SDK will be present in bothdist/credentials.jsanddist/signed-commit.jseven though the latter doesn't use it. This is an intentional trade-off for simplicity and self-containment.- There is a minor patch-level version skew between the direct
@aws-sdk/credential-provider-web-identity@3.972.29and the transitive@3.972.0pulled byclient-secrets-manager. Both patch versions will be bundled, which is harmless for correctness given the minor patch delta.
No bugs found in the changed code.
…ndles - Align @aws-sdk/credential-provider-web-identity 3.972.29 → 3.972.0 to match the rest of the @aws-sdk/* 3.972.0 family. The .29 pin was incidental (introduced in #119 as the version available at the time; no deliberate reason documented). Eliminates the dual-version split in the lockfile and removes @aws-sdk/nested-clients 3.996.19 (the original DOMParser source) from the tree entirely (-15 packages). - Disable tsup sourcemaps. The action is consumed via `uses: docker/cagent-action@v1`, which clones the tagged release including dist/. Sourcemaps would add ~10MB to every consumer clone with no runtime benefit (Node doesn't load them by default). Verified: build clean, 0 DOMParser refs in dist, 30/30 unit tests pass, typecheck clean, no .map files in dist/. Assisted-By: docker-agent
|
/review |
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The fix is correct and well-structured. The root cause (Vite resolving AWS SDK browser exports instead of Node exports) is accurately diagnosed, and the tsup configuration directly addresses it:
platform: 'node'ensures esbuild selects thenodeexport condition from every package, preventing browser-only globals likeDOMParserfrom entering the bundlenoExternal: [/.*/]correctly bundles all dependencies so the action works withoutnode_modulesat runtimesplitting: false+format: 'esm'produces self-contained per-entry-point ESM files- The
outExtension: () => ({ js: '.js' })preserves the.jssuffix expected by the composite action'snode dist/credentials.jsinvocation - Extracting Vitest config to its own
vitest.config.tsis a clean separation; no test regressions - AWS SDK version alignment to 3.972.0 across all packages avoids Smithy runtime version-mismatch issues
No bugs found in the changed code.
Assisted-By: docker-agent
cf0ffe8 to
b2596c9
Compare
Problem
The
Setup credentialsstep was failing in production with:Failing run: https://github.com/docker/cagent-action/actions/runs/25329159765/job/74257978215
Root cause
Vite (the previous bundler) was resolving the AWS SDK's browser export condition instead of the node one. The Smithy XML codec used by
@aws-sdk/client-secrets-managercontains a reference toDOMParser— a browser-only global that does not exist in Node.js. Because the action runs as a composite GHA step (node dist/credentials.js) with nonode_modules, the bundled output was shipping browser code into a pure Node runtime.Fix
Swap Vite → tsup with explicit Node targeting:
platform: 'node'— tells esbuild to resolve thenodeexport condition from every packagetarget: 'node24'— matches the GHA runner Node version; enables modern JS outputnoExternal: [/.*/]— bundles all npm deps (required; action has nonode_modulesat runtime)splitting: false/format: 'esm'— single-file ESM bundles per entry pointsrc/*/index.ts→dist/credentials.js+dist/signed-commit.jsAlso bump
@aws-sdk/client-secrets-manager3.787.0 → 3.972.0 to align with the@aws-sdk/credential-provider-web-identity3.972.x family already in the lockfile, so all SDK packages share a single Smithy runtime instance and avoid version-mismatch issues.vite.config.tsis deleted; its Vitest configuration is extracted into the newvitest.config.ts.Verification
pnpm build— tsup v8.5.1, buildsdist/credentials.js(1.99 MB) +dist/signed-commit.js(148 KB) cleanlypnpm test)DOMParserreferences in dist outputpnpm typecheck+biome cipasscc @derekmisler