Repository navigation
Conversation
Import attributes (`import data from './x.json' with { type: 'json' }`)
were silently stripped at two points in the build pipeline:
1. The SWC transform applied to every bundled JS/TS file drops import
attributes unless `jsc.experimental.keepImportAttributes` is set.
2. esbuild considers import attributes unsupported syntax for the
`es2022` target and removes them from emitted output.
Bundled JSON imports are inlined by esbuild so they were unaffected, but
any JSON import that ends up externalized in the output (relative
project-local paths, bare package specifiers, or dependency-internal
imports like builtin-modules' `./builtin-modules.json`) lost its
attribute and crashed at runtime on Node's ESM loader with
ERR_IMPORT_ATTRIBUTE_MISSING, breaking all local step execution.
Fix by enabling `keepImportAttributes` in the SWC transform and marking
`import-attributes` as supported syntax for every esbuild pass whose
output is loaded directly by Node.
Fixes vercel#3157
Covers both strip points from vercel#3157: - applySwcTransform must emit `with { type: 'json' }` unchanged. - A steps bundle with an externalized project-local JSON import and a dependency-shaped package using a JSON import attribute (mirroring builtin-modules) must keep the attribute in the generated output and load successfully under Node's native ESM loader.
🦋 Changeset detectedLatest commit: ecdd6c4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@Mohith26 is attempting to deploy a commit to the Vercel Labs Team on Vercel. A member of the Team first needs to authorize it. |
|
@Mohith26 We require commits to be signed. Could you squash+sign the PR and force-push the branch? Separate review following soon. |
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
AI review: no blocking issues
| ? { | ||
| plugins: [[swcPluginPath, { mode, moduleSpecifier }]] as [ | ||
| string, | ||
| Record<string, unknown>, |
There was a problem hiding this comment.
AI Review: Note
Correct and minimal fix, and the right two layers: SWC was dropping the attribute before esbuild ever saw it, so setting supported alone would not have helped. Full packages/builders suite is green locally on this branch (38 files / 464 tests).
One thing worth a comment in the code: the as [string, Record<string, unknown>][] cast. It reads like a workaround for experimental.plugins widening once the object is built conditionally rather than inline; a one-liner saying why keeps the next reader from "simplifying" it back into a type error.
| platform: 'node', | ||
| conditions: ['node'], | ||
| target: 'es2022', | ||
| supported: ESBUILD_SUPPORTED_FEATURES, |
There was a problem hiding this comment.
AI Review: Note
ESBUILD_SUPPORTED_FEATURES is applied to 5 of the 6 esbuild invocations in this file; the interim workflow-VM bundle (interimBundleCtx) is left without it. That is correct — the comment at ~L1495 explains that pass deliberately never uses external because the workflow VM (vm.runInContext) has no require(), so every JSON import is inlined and no attribute can reach the output.
Verified empirically on this branch: built the workflow bundle for a workflow file importing both a project-local .json and a dependency whose entry does import data from './data.json' with { type: 'json' }. Both payloads are inlined (dep json present in the output) and no bare .json import survives, so the omission is safe.
Since the asymmetry is not obvious from the diff, a short comment at that call site ("JSON is always inlined here — the VM has no loader, so import attributes cannot appear in this output") would prevent someone adding it later and assuming it was an oversight, or removing it elsewhere by analogy.
| expect(code).toMatch(/with\s*\{\s*type:\s*'json'\s*\}/); | ||
| }); | ||
|
|
||
| it('keeps the attribute on externalized JSON imports so the steps bundle executes', async () => { |
There was a problem hiding this comment.
AI Review: Nit
Coverage is steps-bundle-only. The workflow bundle path takes a different route (interim VM pass + final pass, and no externalization), and I confirmed locally that it inlines JSON rather than emitting attributed imports — worth one test asserting that, so a future change that starts externalizing JSON in the workflow bundle fails loudly instead of producing a bundle the VM can't load.
If there is no reply from him, is it okay if someone (including myself) work on this? Facing a similar problem with |
|
@ruchernchong I'll take over the PR to fix this, thanks for the bump |
SWC and esbuild both dropped `with { type: 'json' }`, so an externalized
JSON import crashed Node's ESM loader with ERR_IMPORT_ATTRIBUTE_MISSING.
Fast discovery also matched serde patterns inside string literals, which
pulled @workflow/builders' serde-checker (and its builtin-modules JSON
import) into every step and workflow bundle.
Supersedes #3160. Fixes #3157.
Co-Authored-By: Mohith Gajjela <109003762+Mohith26@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fixes #3157, reported by @tmatkinson.
Import attributes (
with { type: 'json' }) were stripped at two points in the build pipeline: the SWC transform drops them unlessjsc.experimental.keepImportAttributesis set, and esbuild treats them as unsupported syntax for thees2022target and removes them. Inlined JSON imports were fine, but any JSON import that ends up externalized (relative project paths, bare specifiers, or dependency-internal imports like builtin-modules'./builtin-modules.json) lost its attribute and crashed Node 22's ESM loader withERR_IMPORT_ATTRIBUTE_MISSING, breaking local step execution.Fix: enable
keepImportAttributesin the SWC transform and markimport-attributesas supported syntax for every esbuild pass whose output Node loads directly. Includes a changeset (patch for@workflow/builders).Regression tests cover both stripping points. 232 builders tests pass locally; the new tests fail without the fix.