feat: complete wildcard imports resolution - #608
Conversation
|
Reviewed as the follow-up to my conformance review on #604. Both resolver fixes are exactly right and land where I'd hoped — the src diff matches the patch I validated there, including the function replacer on the new substitution site, which is the easy thing to forget. Fixtures execute correctly under real Node (I ran both), so they're pinned to real semantics rather than just to nft's own expectations. One real coverage gap, one undocumented behavior change, and some nits. Verification I ranI mutation-tested each fix independently — reverted it, rebuilt, ran
So the The gap: the new substitution site has no
|
| result | |
|---|---|
| Node | ERR_MODULE_NOT_FOUND |
main |
resolves fallback/dep/thing.js ← diverges from Node |
| this PR | unresolved + trace warning ← matches Node |
So this is another conformance win, not a regression — Node commits to one bestMatch and never retries a less-specific key (packageImportsResolve). But it's worth (a) a line in the description, since a consumer whose package leaned on that accidental fallback will start seeing new trace warnings after upgrading, and (b) a fixture, since nothing currently locks in "don't fall through" and it'd be easy to reintroduce.
Nits
- The precedence fixture has no
generic/directory.{"#*": "./generic/*.js"}can never resolve, so if precedence regressed and#*won, the test fails becausegeneric/precedence.js.jsis missing rather than because the wrong existing file was picked. Addinggeneric/precedence.js.jsmakes the assertion direct and doesn't change expected output (#*.jsstill wins, so it's never traced). 304/304in the description is306/306on this branch — and onmain, which already has the twoimports-wildcardcases from feat: resolve package import wildcard trailers #604. Looks like the numbers were captured pre-rebase. Worth double-checking the "integration suite excludingsocket.io.js— 88/88" line came from the rebased branch too.- All five new behaviors ride on the same two test cases (
imports-wildcardfrom cwd + root), since the fixture was extended rather than split. Fine and consistent with how the suite is organized, but it does mean a single failure won't tell you which of the five regressed. Not worth restructuring for; just noting it as the tradeoff. - The
patternKeyComparecomparator-consistency point from the last review is untouched, which is the right call — I flagged it as informational and couldn't produce a wrong result from it. Mentioning only so it reads as a decision rather than an oversight. - Deferring the
null-target fallthrough with that rationale is the right call too, and the reasoning in the description is better than what I wrote — "blocking can remove files from traces" is exactly the compatibility risk.
Nothing here blocks except the coverage gap, which is two lines.
|
Addressed in 748f505. Added coverage for a literal |
|
Re-reviewed at 748f505. The coverage gap I flagged is closed and the two nits are addressed — I re-ran the mutation matrix and the external-target replacer is now caught. One item is still outstanding, though: the no-fallthrough fixture doesn't actually exercise no-fallthrough. Now covered ✅
Fixture still executes correctly under real Node, Outstanding: the no-fallthrough fixture is inert
Verified by reintroducing fallthrough — wrapping the new - const resolved = await resolveDependency(...);
- return Array.isArray(resolved) ? resolved : [resolved];
+ try {
+ const resolved = await resolveDependency(...);
+ return Array.isArray(resolved) ? resolved : [resolved];
+ } catch {}306/306 still pass, and the emitted trace is byte-identical — same nine files, same single The fix is to pick a specifier -import('#missing/thing.js').catch(() => {});
+import('#missing/thing.mjs').catch(() => {});-test/unit/imports-wildcard/generic/missing/thing.js.js
+test/unit/imports-wildcard/generic/missing/thing.mjs.js
Worth also softening the description line — "Mutation checks confirmed that the tests fail independently when each resolver fix is reverted, including … no-fallthrough behavior" isn't true for the no-fallthrough case as currently written. Optional, and not this PRDropping the trailer check in Everything else looks good to me. Re-ping me once the |
Summary
importsandexportstargetsimportstargets that point to external packagesContext
Follow-up to #604 and the resolver conformance review in #604 (comment).
Three resolver gaps remained after wildcard trailers were added:
$&,$$,$`, and$'to be interpreted instead of inserted literally.importsmappings only handled./-relative targets, even though Node also permits package targets such as"#dep/*": "depper/*".This uses function replacers at all three target-substitution sites and mirrors the exact
importsbranch's external dependency resolution after wildcard substitution.The
nulltarget fallthrough behavior noted in the review is intentionally not changed here because blocking a target can remove files from traces and warrants a separate compatibility decision. The existingpatternKeyCompareimplementation is also intentionally left unchanged; its comparator-consistency concern is informational and no incorrect wildcard selection has been demonstrated.Regression coverage
The fixtures now cover:
$&wildcard capture for package imports$&capture in both module-sync and fallback export targets$&capture in a wildcard import targeting an external package#lib/*/index.js)#*.jsover#*, with both candidate files presentMutation checks confirmed that the tests fail independently when each resolver fix is reverted, including the external-target function replacer and no-fallthrough behavior.
Validation
pnpm buildpnpm prettier-checkgit diff --checkpnpm exec jest test/unit.test.js --runInBand --silent— 304/304 passed locally on macOS (the platform skips two additional cwd/root cases compared with Linux)