fix(detector): stop silent dependency drops in SEA mode (ESM parse, dynamic import, decorators)#268
Merged
robertsLando merged 5 commits intomainfrom Apr 24, 2026
Merged
Conversation
`detector.parse()` called babel with the default `sourceType: 'script'`,
so SEA-mode walker runs over `import.meta` / top-level `await` failed
to parse and silently skipped the file's dependency traversal. Thread
`isESMFile(record.file)` through `stepDetect` → `detect()` → `parse()`
so ESM files get `sourceType: 'module'`. Also teach the visitor to
recognize `import('literal')` `CallExpression`s so bundler-emitted
dynamic imports are bundled like static ones.
Third-party sources that ship raw `@decorator` syntax (fontkit, older MobX / Nest builds) tripped the same silent-drop failure mode as #264: babel.parse threw, `detect()` logged a warning, and the file's dependency graph was dropped. Enable `decorators-legacy` in both the walker's detector and the ESM-transformer parse calls so these sources parse cleanly and their requires/imports get bundled. Extend test-94 with a decorator fixture walked via `pkg.scripts`.
There was a problem hiding this comment.
Pull request overview
This PR fixes SEA-mode dependency detection regressions by improving Babel parsing and expanding the detector’s import matching so ESM graphs aren’t silently dropped after parse failures.
Changes:
- Parse walked files as ESM modules when
isESMFile(...)indicates ESM, and enabledecorators-legacyin both the detector and ESM transformer. - Detect and walk dynamic
import('literal')the same way as static imports /require(...). - Add a SEA-focused regression test covering
import.meta, top-levelawait import(...), and decorator syntax parsing.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
test/test.js |
Registers the new SEA regression test in the host-only test list. |
test/test-94-sea-esm-import-meta/main.js |
New test asserting no “Babel parse has failed” warnings and validating SEA binary output. |
test/test-94-sea-esm-import-meta/app/package.json |
ESM fixture app (type: module) + pkg.scripts to include decorator fixture. |
test/test-94-sea-esm-import-meta/app/index.mjs |
Fixture using import.meta and a dynamic import('./lib/dyn.mjs'). |
test/test-94-sea-esm-import-meta/app/lib/helper.mjs |
Static import target fixture. |
test/test-94-sea-esm-import-meta/app/lib/dyn.mjs |
Dynamic import target fixture. |
test/test-94-sea-esm-import-meta/app/lib/decorated.js |
Decorator-syntax fixture to ensure parser plugin coverage. |
lib/walker.ts |
Threads isESMFile(record.file) into detector.detect(...) so parsing uses the correct source type. |
lib/esm-transformer.ts |
Enables decorators-legacy so transformer parsing doesn’t fail on decorator syntax. |
lib/detector.ts |
Adds ESM-aware parsing, enables decorators-legacy, and detects dynamic import('literal'). |
eslint.config.js |
Ignores the decorator-syntax fixture from linting. |
Guard visitorDynamicImport against `import(0)` / `import(true)` so a numeric or boolean literal can't flow through the walker as an alias and crash downstream string checks (e.g. isBuiltin's moduleName.startsWith). Addresses Copilot review feedback on PR #268. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
robertsLando
added a commit
to yao-pkg/pkg-action
that referenced
this pull request
Apr 24, 2026
pkg v6.19.0 (yao-pkg/pkg#263, closes #262) landed config support for the CLI-only build flags, and ships the SEA-mode detector fix (yao-pkg/pkg#268) that caused the prior OOM at bytecode-compile time. - claude-code-smoke: add `compress: 'Zstd'` alongside `sea: true` in package.json#pkg; bump pkg-version input ~6.18.0 → ~6.19.0. - Default `pkg-version` input bumped ~6.16.0 → ~6.19.0 — the whole config-as-source-of-truth direction of this branch requires it. - Regenerated action.yml / packages/build/action.yml / docs/inputs.md. - STATUS.yaml: upstream-dependency RESOLVED; Zstd gap dropped; e2e-status lists claude-code-smoke; ci-status bumped to 227 tests. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
robertsLando
added a commit
to yao-pkg/pkg-action
that referenced
this pull request
Apr 24, 2026
…13) * refactor!: drop pkg-CLI mirror inputs, own only action-layer surface Pkg-specific knobs (mode, node-version, compress-node, fallback-to-source, public, public-packages, options, no-bytecode, no-dict, debug, extra-args) are removed from the action input surface. Users express them in their pkg config (.pkgrc, pkg.config.{js,ts,json}, or package.json pkg field) instead. The action keeps only action-layer concerns that do not exist in pkg config: config path, entry, targets, pkg-version/pkg-path, plus the archive, signing, windows-metadata, and performance groups. Motivation: mirroring pkg's CLI forced the action to grow a new input each time pkg added a flag, and to maintain back-compat on every pkg release. By letting pkg own its own schema we decouple the action from pkg's evolution. Docs updated: README gets a new "pkg configuration" section with a migration example; architecture.md §3/§6 note the scoped surface; STATUS.yaml records the dropped-inputs list and migration path. BREAKING CHANGE: the listed inputs are no longer accepted. Setting them now triggers the unknown-input warning and the value is ignored. Migrate into your pkg config file. * review: link STATUS from architecture, cover pkg-version/pkg-path in tests Address PR-review suggestions: - architecture.md §6 no longer duplicates the dropped-input list; points readers at STATUS.yaml#input-surface-slim as the single source. - inputs.test.ts: extend the defaults test with `pkgPath` and add a positive round-trip test for `pkg-version` + `pkg-path`. These remain live inputs and deserve the same explicit coverage the rest of BuildInputs has. 223/223 unit tests, lint clean. * feat: add config-inline input for file-less pkg config New optional input `config-inline` accepts a JSON string carrying the pkg config directly in the workflow. The orchestrator writes it to `${invocationDir}/pkg-config.inline.json` and forwards that path to pkg via `--config`. Mutually exclusive with `config`; the pair is validated at parse time alongside a JSON-object syntax check so a typo fails before pkg runs. Motivation: users with a trivial pkg config (entry + a couple of knobs) no longer need to commit a separate .pkgrc.json just to customize the build. Keeps the "pkg owns its schema" property — users still write pkg config keys (camelCase), we just let them write them inline. Not masked as a secret — README adds a note warning users against embedding secrets in the value. Docs: README gains an "Inline config" subsection with an example; action.yml + docs/inputs.md regenerated. Tests: four new unit tests cover the happy path, mutual exclusion, invalid JSON, and non-object JSON (string/array/null). 227/227 pass. * review(copilot): use correct camelCase pkg-config keys in warnings/docs Three nits from the Copilot review on PR #13: - targets.ts cross-compile warnings referenced `fallback-to-source` (kebab, like the retired CLI flag); pkg config uses `fallbackToSource` (camelCase). Reword both messages to point at the actual config key. - README "pkg configuration" example was fenced as jsonc with a `//` comment and trailing comma but labeled `.pkgrc.json` — .pkgrc.json is strict JSON, so a copy/paste would fail. Move the filename into the prose, fence as json, drop the comment + trailing comma. Unit tests + lint green. Matrix bundle rebuilt to pick up the new warning strings. * e2e(claude-code): move pkg-layer knobs from action inputs to pkg config Main (PR #12) wired the claude-code-smoke job through the action using `mode: sea` and `compress-node: Zstd` inputs. Those inputs no longer exist on this branch, so move both values into the package.json#pkg field that the fixture-prep step already injects. No behavior change on main; on this branch the e2e now exercises the pkg-config path the slim input surface forces users onto. * e2e(claude-code): use sea:true config, acknowledge CLI-only flag gap The merge from main brought in the claude-code-smoke job, which previously drove pkg via `mode: sea` + `compress-node: Zstd` action inputs. On this branch those inputs are dropped. The prior merge-conflict resolution moved both into `package.json#pkg` under the assumption that pkg config would pick them up. That assumption was wrong: - `mode: 'sea'` is not a pkg config key — the correct key is `sea: true`. (This was a user-visible bug; pkg silently ignored `mode` and ran standard-mode, which OOM'd trying to bytecode-compile claude-code's ESM bundle on macos-arm64.) - `compressNode` / `compress` is not a pkg config key at all. `--compress` is CLI-only today on @yao-pkg/pkg. No config equivalent exists. Fix the e2e: - Write `sea: true` into package.json#pkg so SEA is actually enabled. - Drop the Zstd request — there is no way to express it without the dropped CLI input. The job now exercises SEA + tar.gz; the Zstd branch is intentionally deferred until upstream lands config support. - Rename + retone the job's comment block accordingly. Also add an upstream-dependency note to STATUS.yaml and a draft issue body at docs/upstream-pkg-config-issue.md. The draft asks yao-pkg/pkg to accept the currently-CLI-only build flags (compress, fallbackToSource, public, publicPackages, options, noBytecode, noDict, debug, signature) in the config file — that's the piece that makes this PR's scope defensible for the full input set, not just for SEA. 227 unit tests pass; lint + prettier clean. * status: link upstream pkg-config tracking issue (yao-pkg/pkg#262) Issue is open upstream asking @yao-pkg/pkg to accept the currently-CLI-only build flags (compress, fallbackToSource, public, publicPackages, options, noBytecode, noDict, debug, signature) in the config file — the piece that makes this PR's drop-the-CLI-mirror direction fully viable. Replace the local draft with the issue URL in STATUS.yaml#upstream-dependency. * e2e(claude-code): enable Zstd via pkg config; bump pkg → ~6.19.0 pkg v6.19.0 (yao-pkg/pkg#263, closes #262) landed config support for the CLI-only build flags, and ships the SEA-mode detector fix (yao-pkg/pkg#268) that caused the prior OOM at bytecode-compile time. - claude-code-smoke: add `compress: 'Zstd'` alongside `sea: true` in package.json#pkg; bump pkg-version input ~6.18.0 → ~6.19.0. - Default `pkg-version` input bumped ~6.16.0 → ~6.19.0 — the whole config-as-source-of-truth direction of this branch requires it. - Regenerated action.yml / packages/build/action.yml / docs/inputs.md. - STATUS.yaml: upstream-dependency RESOLVED; Zstd gap dropped; e2e-status lists claude-code-smoke; ci-status bumped to 227 tests. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * chore: rebuild dist/ bundles after pkg-version bump Rebuild packages/{build,windows-metadata}/dist/index.mjs so the committed bundles match the ~6.19.0 default written into packages/core/src/inputs.ts. Drift gate caught it. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * review: correct pkg-config keys, mask config-inline, extract materialize helper Address four items from the self-review on PR #13: - README examples used `mode: "sea"` / `compressNode: "Brotli"` — neither exists in @yao-pkg/pkg's config schema. The authoritative FLAG_SPECS in lib/config.ts (and the docs-site configuration reference) show `sea: true` and `compress: "..."`. Fixed both examples so a copy-paste works. - `config-inline` carried user-authored JSON but was not masked. Mark the spec with `secret: true` so parseInputs registers the raw value via core.setSecret — exact-match redaction in logs costs nothing given pkg doesn't echo the config blob. README warning softened accordingly (still flags the on-disk temp file). - Extract the config-inline "bytes to disk" step out of the orchestrator into `materializePkgConfigInline` in fs-utils, alongside `PKG_CONFIG_INLINE_FILENAME`. Four new unit tests cover pass-through, both-undefined, inline-writes-and-returns-path, and inline-wins-over-config. Orchestrator is now a two-liner that just logs the resulting path. - BASE_BUILD fixture in pkg-runner.test.ts still pinned pkgVersion to the old `~6.16.0` default; bump to `~6.19.0` to match parseInputs. Regenerated action.yml + docs/inputs.md (secret flag on config-inline doesn't affect the YAML shape but the description changed). Rebuilt packages/{build,windows-metadata}/dist/index.mjs. Tests 231/231 pass (was 227/227), typecheck + lint + prettier green. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> --------- Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
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.
Summary
Three related fixes to
lib/detector.ts(and the ESM transformer), all of which cure the same user-visible symptom: pkg logged aBabel parse has failedwarning and then silently dropped the file's dependency graph, forcing users to enumerate missing files inpkg.scripts. Most visible in SEA mode, where the ESM→CJS transform is skipped and the walker's detector is the only dependency pass.detector.parse()called babel with the defaultsourceType: 'script', so.mjs/type:modulefiles usingimport.metaor top-levelawaitfailed to parse. ThreadisESMFile(record.file)fromwalker.stepDetectthroughdetect()intoparse()and setsourceType: 'module'for ESM files. Per-file detection uses the existingisESMFilehelper (extension + nearestpackage.json#type), so CJS files keep script-mode parsing.import('literal'). TeachvisitorSuccessfulto matchCallExpressionnodes whose callee isImportwith a literal first arg, returningALIAS_AS_RESOLVABLE. Bundler-emitted chunk splits (await import('./chunk-xxx.js')) now get bundled likerequire()/ staticimport … from …. Non-literal dynamic imports stay silent.decorators-legacybabel plugin. Third-party sources that ship raw@decoratorsyntax (fontkit, older MobX / Nest builds) tripped the same parse-failure → silent-drop flow. Enabling the plugin in the detector and inlib/esm-transformer.tslets them parse cleanly. Covers both class and method decorator positions, which is what 99% of published code uses.sourceType: 'unambiguous'was rejected for the ESM case: a file whose only ESM marker isawait import(…)has no static import/export syntax, so unambiguous falls back to script mode and the original parse failure returns. The package.jsontypefield is the authoritative signal.lib/detector.tsalso gets concise JSDoc on every visitor and helper so the next contributor doesn't have to reverse-engineer them from the AST.Closes #264 (follow-ups filed at #269 for other detector shapes still silently dropped).
Test plan
yarn build,yarn linttest-94-sea-esm-import-meta: SEA build of atype:moduleapp withimport.meta.url, a staticimport, a dynamicimport('./lib/dyn.mjs'), and a decorator-using file (lib/decorated.js) walked viapkg.scripts. AssertsBabel parse has failednever appears in pkg output, and the binary prints the imported values — proving all the walked files made it into the snapshot.ERR_MODULE_NOT_FOUNDonhelper.mjs; stashing the dynamic-import visitor → same miss ondyn.mjs; stashing the plugin change →Babel parse has failed: This experimental syntax requires enabling one of the following parser plugin(s)emitted fordecorated.js.test-85-sea-enhanced,test-87-sea-esm,test-91-sea-esm-entry,test-92-sea-tla,test-51-esm-import-meta,test-52-esm-internal-imports,test-53-esm-nested-imports— all green.