fix(detector): pick up six more silent-drop shapes (re-exports, new URL, multi-arg path.join, path.resolve(__dirname,…), createRequire aliases, import.meta.resolve)#272
Open
robertsLando wants to merge 2 commits intomainfrom
Conversation
…-arg path.join, path.resolve(__dirname,...), createRequire aliases, import.meta.resolve Follow-up to #268. Six more detector shapes that silently dropped their target — most visible in SEA mode where the ESM→CJS transform is skipped and the detector is the only dependency pass. - ESM re-exports (`export * from "lit"` / `export { x } from "lit"` / `export * as ns from "lit"`). visitorImport only matched ImportDeclaration; ExportAllDeclaration / ExportNamedDeclaration with a `.source` now go through a parallel visitorReExport. - `new URL("./rel", import.meta.url)` — canonical ESM sibling-asset idiom. Treated as ALIAS_AS_RELATIVE; only matches when the base is literally `import.meta.url` so we don't synthesize paths from arbitrary URL bases. - multi-arg `path.join(__dirname, "a", "b", …)` and `path.resolve(__dirname, "a", "b", …)`. Old code required exactly 2 args; now any literal-only segment list joins via path.posix.join to a single posix alias. Bails on non-literal segments to avoid guessing. - `path.resolve(__dirname, "lit")`. Same intent as `path.join`; also silences the spurious "ambiguous resolve" warning that visitorUseSCWD used to emit for this exact shape (the call is now bundled, so the warning would contradict our own action). - `createRequire(import.meta.url)("./foo")` and the aliased form `const r = createRequire(...); r("./foo")`. detect() now pre-scans the AST for VariableDeclarator/AssignmentExpression bindings whose RHS is a `createRequire(...)` call, threads the bound names through the visitor as a 3rd arg, and visitorRequire / visitorRequireResolve treat them as require-equivalent. - `import.meta.resolve("lit")` — modern ESM resolver API. Same ALIAS_AS_RESOLVABLE handling as `require.resolve`. The issue's claim about `require.resolve("lit", { paths: [...] })` silently dropping was verified to be wrong — `valid2(null)` already accepts the ObjectExpression-as-2nd-arg case, so existing code bundles the target correctly. No change there. Walker.ts forwards the new `requireAliases` Set from detect()'s visitor signature into visitorSuccessful so per-file alias state is applied. Closes #269 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Pull request overview
This PR extends lib/detector.ts so the walker’s dependency detection (especially important in SEA mode) recognizes additional common ESM/Node patterns that previously resulted in silently dropped dependencies.
Changes:
- Add new detector matchers for ESM re-exports,
new URL("./rel", import.meta.url),import.meta.resolve("..."), and improvedpath.join/resolve(__dirname, ...)handling (including multi-arg joins). - Add a pre-scan for
createRequire(...)aliases and thread the resultingrequireAliasesset through detection so aliasedrequirecalls can be recognized. - Add unit tests covering the new shapes and update
lib/walker.tsto forward the new alias context intovisitorSuccessful.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
lib/detector.ts |
Adds new AST matchers, createRequire alias pre-scan, and extends visitorSuccessful to use per-file requireAliases. |
lib/walker.ts |
Forwards requireAliases from detect() into visitorSuccessful so runtime behavior matches per-file alias state. |
test/unit/detector.test.ts |
Adds unit coverage for the new detector shapes and alias threading behavior. |
- visitorNewURL & visitorImportMetaResolve: tighten the MetaProperty
guard. `MetaProperty` also represents `new.target`, so checking for the
type alone matched `new URL("./x", new.target.url)` and a hypothetical
`new.target.resolve(...)`. Added isImportMeta() that asserts the
meta/property pair is exactly `import`/`meta`.
- collectRequireAliases: drop the deep traversal in favor of a single
pass over `program.body` and only accept top-level `const` bindings.
Pre-fix the rule was scope-blind and would shadow-leak `r` from an
inner function into the outer scope. Top-level-`const`-only covers
the canonical idiom (`const r = createRequire(import.meta.url)` at
module top) without needing real scope tracking.
- visitorUseSCWD: only skip the warning when *every* arg after
__dirname is a literal — i.e. when visitorPathJoin actually claims
the call. Dynamic shapes like `path.resolve(__dirname, x)` now warn
again, since visitorPathJoin bails on the non-literal segment and
the diagnostic is the only signal the user gets.
- visitorNonLiteral / visitorMalformed (and their helpers): thread
`requireAliases` so aliased dynamic / malformed requires
(`r(x)` / `r()`) emit the same warnings as the literal `require`
forms instead of being silently dropped.
Five new unit tests cover the tightened behaviors; verified each fails
without this commit and passes with it.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
lib/detector.ts:538
visitorPathJoinnow matches bothpath.joinandpath.resolve, but thetestprintable form always renders aspath.join(__dirname, ...), which is misleading forpath.resolvecases and can make future unit tests/debug output inaccurate. Consider returning which method matched (join vs resolve) fromvisitorPathJoinand using it in theforge(...)pattern.
if (was) {
if (test) {
return forge('path.join(__dirname{c1}{v1})', was);
}
Comment on lines
+132
to
+141
| * True if `n` is a `createRequire(...)` call. Used both to detect the direct | ||
| * `createRequire(import.meta.url)('./foo')` invocation pattern and to seed the | ||
| * alias set with names bound to its result. | ||
| */ | ||
| function isCreateRequireCall(n: babelTypes.Node | null | undefined) { | ||
| return ( | ||
| !!n && | ||
| babelTypes.isCallExpression(n) && | ||
| babelTypes.isIdentifier(n.callee) && | ||
| n.callee.name === 'createRequire' |
Comment on lines
+748
to
+757
| const firstArg = n.arguments[0]; | ||
|
|
||
| if ( | ||
| firstArg && | ||
| babelTypes.isIdentifier(firstArg) && | ||
| firstArg.name === '__dirname' && | ||
| n.arguments.length >= 2 && | ||
| n.arguments.slice(1).every((a) => isLiteral(a)) | ||
| ) { | ||
| return null; |
Comment on lines
+827
to
+851
| /** | ||
| * Pre-scan pass: collects identifiers bound at the *module top level* to | ||
| * `createRequire(…)` so the main traversal can recognize `r("./foo")` (where | ||
| * `r` was assigned from `createRequire`) as a require-equivalent. | ||
| * | ||
| * Restricting to top-level `const` declarations is deliberate: a deep walk | ||
| * would also pick up bindings inside inner functions or shadowed scopes, | ||
| * which would falsely flag unrelated `r(...)` calls in other scopes as | ||
| * requires. The canonical `const r = createRequire(import.meta.url)` idiom is | ||
| * always file-top, so the safe-and-narrow rule covers the real-world cases | ||
| * without scope tracking. | ||
| */ | ||
| function collectRequireAliases(ast: babelTypes.File) { | ||
| const names = new Set<string>(); | ||
|
|
||
| for (const stmt of ast.program.body) { | ||
| if (!babelTypes.isVariableDeclaration(stmt) || stmt.kind !== 'const') { | ||
| continue; | ||
| } | ||
|
|
||
| for (const decl of stmt.declarations) { | ||
| if (babelTypes.isIdentifier(decl.id) && isCreateRequireCall(decl.init)) { | ||
| names.add(decl.id.name); | ||
| } | ||
| } |
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
Follow-up to #268. Six more detector shapes that silently dropped their target — most visible in SEA mode where the ESM→CJS transform is skipped and the detector is the only dependency pass.
export * from "lit",export { x } from "lit",export * as ns from "lit".visitorImportonly matchedImportDeclaration;ExportAllDeclaration/ExportNamedDeclarationwith.sourcenow go through a parallelvisitorReExport.new URL("./rel", import.meta.url)— canonical ESM sibling-asset idiom. Treated asALIAS_AS_RELATIVE. Only matches when the base is literallyimport.meta.urlso we don't synthesize paths from arbitrary URL bases.path.join(__dirname, "a", "b", …)andpath.resolve(__dirname, "a", "b", …). Old code required exactly 2 args; now any literal-only segment list joins viapath.posix.jointo a single posix alias. Bails on non-literal segments to avoid guessing.path.resolve(__dirname, "lit")— same intent aspath.join; also silences the spurious "ambiguous resolve" warningvisitorUseSCWDused to emit for this exact shape (the call is now bundled, so the warning would contradict our own action).createRequire(import.meta.url)("./foo")(direct invocation) andconst r = createRequire(...); r("./foo")(aliased form).detect()now pre-scans the AST forVariableDeclarator/AssignmentExpressionbindings whose RHS is acreateRequire(...)call, threads the bound names through the visitor as a 3rd arg, andvisitorRequire/visitorRequireResolvetreat them as require-equivalent.import.meta.resolve("lit")— modern ESM resolver API. SameALIAS_AS_RESOLVABLEhandling asrequire.resolve.The issue's claim that
require.resolve("lit", { paths: [...] })silently drops was verified to be wrong —valid2(null)already accepts theObjectExpression-as-2nd-arg case (becauseisLiteral(ObjectExpression)is false,v2is set tonull, andvalid2(null)returnstrue), so existing code bundles the target correctly. No change there.walker.tsforwards the newrequireAliasesSet fromdetect()'s visitor signature intovisitorSuccessfulso per-file alias state is applied.Closes #269
Test plan
yarn build,yarn lint— cleantest/unit/detector.test.tscovering every new shape (re-exports including named/namespace, multi-arg join/resolve, non-literal segment bail-out,new URLwith non-import.meta.urlbase rejected,import.meta.resolve, direct + aliasedcreateRequire, alias propagating tor.resolve, sanity check that unrelated identifiers don't get treated as require aliases,path.resolve(__dirname, …)no longer triggeringvisitorUseSCWD).lib/detector.ts+lib/walker.ts→ 12 failures, all and only the new tests. Restored → 211/211 pass.🤖 Generated with Claude Code