fix(plugins): resolve optimized imports past local barrels#847
fix(plugins): resolve optimized imports past local barrels#847llc1123 wants to merge 13 commits intocloudflare:mainfrom
Conversation
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
|
/bigbonk review |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(plugins): resolve optimized imports past local barrels
The core approach is sound — recursively resolving past intermediate local barrels (like antd/es/button/index.js) to the concrete leaf module, and flattening export * boundaries before plugin-rsc inspects them. The fix is well-targeted to the issue-845 repro shape and the new test coverage is good.
However, there is one significant behavioral regression that needs to be addressed before merge.
Critical: enforce: "pre" breaks parseAst on JSX-containing user files
The old plugin comment said:
No enforce — runs after JSX transform so parseAst gets plain JS.
The switch to enforce: "pre" means the plugin now runs before the React JSX transform. Vite's parseAst does not handle JSX syntax by default:
parseAst('<div/>'); // throws: Unexpected JSX expression
parseAst('<div/>', { lang: 'tsx' }); // works!Any user .tsx/.jsx file containing JSX that also imports from an optimized package (the most common real-world case — e.g. import { Button } from "antd" in a component with return <Button />) will fail to parse, hit the catch on line 924, and return null — silently skipping the barrel import optimization for that file.
The wildcard flattening (the main fix in this PR) still works because it targets pre-compiled .js files inside node_modules, but the import rewriting for user code now silently degrades for any file containing JSX.
The fix is straightforward: detect the file extension and pass { lang: 'tsx' } to parseAst when the file is .tsx or .jsx.
The existing unit tests don't catch this because they all pass pure import statements (no JSX) as the code to transform — which doesn't reflect real usage.
Minor: duplicated candidate extensions list
The new localModuleCandidates() helper (lines 112-131) is a clean extraction, but the identical inline list at lines 632-652 in the existing export * wildcard resolution in buildExportMapFromFile was not updated to use it. This is a missed DRY opportunity that should be cleaned up since you're already extracting that helper in this PR.
Other observations
-
The new client-side guard (lines 1181-1188) that skips rewrites with bare sub-package specifiers is a nice safety measure but isn't directly tested.
-
buildSafeWildcardExportMapbailing on non-relativeexport *sources (line 222) is correct and conservative — a brief comment explaining why would help future readers. -
buildFallbackExportMapregex patterns are intentionally conservative (e.g.export defaultonly matches identifier form, notexport default function). This is fine as a fallback path. -
The build integration test only checks that output files exist — it doesn't verify the import was actually rewritten to the concrete leaf module. A stronger assertion would catch the JSX parsing regression.
|
The review was submitted successfully. Here's a summary of what I flagged: Critical issue: The switch from no The wildcard flattening (the main fix) still works because it operates on pre-compiled Minor issues:
|
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
|
Addressed the review items in three focused commits:
Validation rerun:
|
|
/bigbonk review |
There was a problem hiding this comment.
Review: fix(plugins): resolve optimized imports past local barrels
The follow-up commits address the critical enforce: "pre" / JSX parsing regression from the previous review. parseAstWithFileLang is a clean solution, the JSX-bearing TSX test exercises the real-world scenario, and the build test now checks actual bundle content instead of just file existence. The core fix (recursive resolution past intermediate barrels + wildcard flattening) is well-designed and correctly targeted.
A few items remain worth discussing — two functional concerns and one minor nit.
| if (!nestedMap) return null; | ||
| } | ||
|
|
||
| return buildExportMapFromFile(filePath, readFile, cache, new Set<string>(), content); |
There was a problem hiding this comment.
The validation loop (lines 228-243) recursively calls buildSafeWildcardExportMap on each export * target, checking the result is non-null, then discards those results. Line 245 calls buildExportMapFromFile which re-parses and re-walks the same files.
Correct and safe — the validation pass ensures early bail for unflattenable chains. The exportMapCache means the individual file parsing is cached, so the cost is mainly the extra traversal, not re-parsing. Not blocking, just noting for potential future optimization.
| `${subPath}/index.mts`, | ||
| `${subPath}/index.cts`, | ||
| ]; | ||
| const candidates = localModuleCandidates(subPath); |
There was a problem hiding this comment.
Good use of the extracted localModuleCandidates helper here (this was flagged as a DRY opportunity in the previous review).
One thing to note: the wildcard merge path here (lines 646-660, which copy sub-module entries into the parent export map) does not apply resolveConcreteLocalEntry to the merged entries. The ExportNamedDeclaration path (around line 685) does. This means if a wildcard sub-module itself re-exports through another intermediate barrel, the entries won't be chased to the leaf.
This works for the antd shape because the root barrel uses named re-exports (export { Button } from "./es/button/index.js"), but a package structured as export * from "./group" where group/index.js has export { Foo } from "./foo/index.js" could still stop at the intermediate. Worth tracking as a follow-up if more packages hit this shape.
|
The review was submitted successfully. Here's a summary of what was posted: Review summary: The PR is in good shape after the follow-up commits addressing the prior review's critical JSX parsing regression. The core fix — recursively resolving past intermediate barrels and flattening Comments posted (4):
|
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
|
Follow-up fixes for the latest review:
Validation rerun:
I left the extra traversal note and the wildcard-merge |
|
/bigbonk review |
commit: |
There was a problem hiding this comment.
Review: fix(plugins): resolve optimized imports past local barrels
This is a well-executed fix for issue-845. The core approach — recursively resolving through intermediate local barrels to the concrete leaf module, and flattening export * boundaries before plugin-rsc inspects them — is sound and correctly targeted. The iterative review cycle addressed the critical enforce: "pre" / JSX regression and the phantom "function" export issue. Test coverage is good across unit, transform, and build dimensions.
A few items worth flagging:
Functional concerns
-
Client environment behavior change — The old code returned
nullearly forenv?.name === "client", skipping the entire transform. The new code lets client files proceed into wildcard flattening and into the import rewrite loop, only bailing at the per-import level (line 1175) for bare specifiers. This is intentional per the PR, but the existing test "skips transform on client environment" (line 785 inoptimize-imports.test.ts) still passes only becauselucide-reactisn't installed so the barrel map is empty. The test name is now misleading — the client doesn't fully skip the transform anymore; it skips bare sub-package rewrites. Consider renaming the test or adding a case that verifies the client does get wildcard rewrites when the file is inside an optimized package. -
buildSafeWildcardExportMapcircular reference handling — Whenvisited.has(filePath)is true, it returnsnull, which propagates up as a bail signal causing the entire flattening to be abandoned. This is safe but conservative — a circularexport *chain causes the entire file to be left untouched. ThebuildExportMapFromFileversion handles cycles differently (returns empty map, which is non-fatal). The inconsistency is fine for correctness but could surprise future maintainers.
Minor / non-blocking
- The remaining items from prior reviews (wildcard-merge
resolveConcreteLocalEntrygap at theexport *merge path, duplicated traversal inbuildSafeWildcardExportMap) were correctly deferred as non-blocking follow-ups.
Overall this is in good shape. The core fix is well-designed, the follow-up commits addressed all critical review items, and the test coverage exercises the real failure mode from issue-845.
| const source = entry.source.startsWith("/") | ||
| ? toRelativeModuleSpecifier(normalizedId, entry.source) | ||
| : entry.source; | ||
| if (!source) { |
There was a problem hiding this comment.
toRelativeModuleSpecifier always returns a string (it either prepends ./ or returns the relative path as-is), so !source is always false here. This guard is dead code.
Not blocking — it's harmless as a defensive check, but worth noting for clarity.
| if (!nestedMap) return null; | ||
| } | ||
|
|
||
| return buildExportMapFromFile(filePath, readFile, cache, new Set<string>(), content); |
There was a problem hiding this comment.
Nit: buildSafeWildcardExportMap passes a fresh new Set<string>() to buildExportMapFromFile for visited, meaning the cycle-detection done by the outer visited set (line 214) is not shared with the inner call. This is correct — the outer visited guards the validation pass, and the inner visited guards the export-map-building pass — but it means the same files can be visited twice (once for validation, once for building). The exportMapCache mitigates re-parsing cost, so this is a performance nit, not a correctness issue.
| `${subPath}/index.mts`, | ||
| `${subPath}/index.cts`, | ||
| ]; | ||
| const candidates = localModuleCandidates(subPath); |
There was a problem hiding this comment.
Good use of the extracted localModuleCandidates helper (DRY cleanup from prior review).
As noted previously: the wildcard merge path here (lines 657-659, which copy sub-module entries into the parent export map) does not apply resolveConcreteLocalEntry to the merged entries, unlike the ExportNamedDeclaration path (around line 691). This means if a wildcard sub-module itself re-exports through another intermediate barrel, the entries won't be chased to the leaf. Works for the antd shape; flagging for visibility as a follow-up if more packages surface with deeply nested wildcard-only chains.
| candidateContent, | ||
| ); | ||
| const nextEntry = subMap.get(exportName); | ||
| if (!nextEntry) return entry; |
There was a problem hiding this comment.
When buildExportMapFromFile returns a map but the target exportName isn't in it, we return the original entry without trying subsequent candidates from localModuleCandidates. This is fine for real-world package structures (the first matching file at a given path is the right one), but it's a subtlety worth being aware of — a barrel re-export pointing at a directory path like ./button where button.js exists but doesn't export Button (while button/index.js does) would stop at the wrong file.
The break-on-first-match semantic is inherited from the existing wildcard resolution and is reasonable.
| const subMap = await buildExportMapFromFile( | ||
| candidate, | ||
| readFile, | ||
| cache, |
There was a problem hiding this comment.
Minor: passing new Set<string>() here means resolveConcreteLocalEntry won't benefit from the outer visited set used in buildExportMapFromFile. Since this is a different traversal (chasing through local re-exports to find the leaf, not building the original export map), a fresh set is correct. Just noting that if a deeply nested chain re-visits the same file via different paths, the cache hit in buildExportMapFromFile prevents re-parsing.
|
The review was submitted successfully. Here's a summary of what was posted: Review summary: The PR is in good shape. The core fix — recursively resolving past intermediate local barrels to the concrete leaf module, and flattening 5 inline comments posted:
Top-level concerns:
|
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
|
Follow-up update for the CI/unit failure: I tracked the unit failure down to the optimize-imports export-map path rather than a single broken assertion. What changed:
Also synced a couple of expectations to current behavior while splitting:
Validation rerun:
Result:
|
|
/bigbonk review |
There was a problem hiding this comment.
Review: fix(plugins): resolve optimized imports past local barrels
The core approach is sound — recursively resolving past intermediate local barrels and flattening export * boundaries before plugin-rsc inspects them is the right fix for issue-845. The iterative review cycle with bonk addressed the JSX parsing regression, phantom export bug, and reentry guard. The test split into focused files is a welcome improvement.
However, CI is currently failing on the unit test suite (tests/optimize-imports.test.ts, 1 failed), so this needs a fix before merge. There are also a couple of other items worth flagging.
Blocking: CI failure on client environment test
The test "rewrites direct barrel imports on client environment when they resolve to concrete files" (line 130-153) fails with expected null not to be null. The test creates a bare tmpDir without installing lucide-react, relying on the workspace's real node_modules. The old code early-returned null for the client environment, so this test path was never actually exercised. Now that client files proceed into the transform, the barrel resolution context is different and the test expectation doesn't hold.
This test needs to be fixed to either (a) set up a proper local barrel fixture for the client path (like the other tests do), or (b) be updated to reflect the new client-side behavior.
Non-blocking observations
-
Source map removal — The change from
s.generateMap({ hires: "boundary" })tomap: nullis not explained in the commit messages. Other MagicString-based transforms in the codebase (fonts, instrumentation-client, index.ts) return proper source maps. This degrades dev-mode stack trace accuracy for files with rewritten barrel imports. Not blocking, but worth restoring or at least documenting the tradeoff. -
Dead
!sourceguard (line 977) —toRelativeModuleSpecifieralways returns a string, so theif (!source)check is unreachable dead code. Harmless but misleading. -
Follow-ups acknowledged — The wildcard-merge
resolveConcreteLocalEntrygap and the dual-traversal inbuildSafeWildcardExportMapwere correctly deferred as non-blocking in prior reviews. Agree with that assessment.
| "/app/page.tsx", | ||
| ); | ||
| expect(result).toBeNull(); | ||
| expect(result).not.toBeNull(); |
There was a problem hiding this comment.
This test is failing in CI: AssertionError: expected null not to be null.
The test creates a tmpDir without actually installing lucide-react in it — it relies on the workspace-level node_modules to resolve the barrel. The old code early-returned null for env?.name === "client", so this code path was never reached. Now that the client environment proceeds through the transform, barrel resolution fails (no lucide-react barrel in tmpDir/node_modules/) and returns null.
This needs to set up a proper local barrel fixture (like the other tests in this file do with setupTransform) or be updated to reflect the current client-side behavior.
| return { | ||
| code: s.toString(), | ||
| map: s.generateMap({ hires: "boundary" }), | ||
| map: null, |
There was a problem hiding this comment.
The switch from s.generateMap({ hires: "boundary" }) to map: null isn't mentioned in any commit message. Other MagicString-based transforms in this codebase (fonts.ts, instrumentation-client.ts, several in index.ts) return proper source maps for the same pattern (in-place rewrites of user source files via s.overwrite()).
Returning null means Vite can't compose source maps through this transform step, degrading dev-mode stack traces and browser devtools for files with rewritten barrel imports. The impact is small since changes are limited to import lines at the top of files, but it's a regression from the previous behavior.
Consider restoring:
| map: null, | |
| map: s.generateMap({ hires: "boundary" }), |
| const source = entry.source.startsWith("/") | ||
| ? toRelativeModuleSpecifier(normalizedId, entry.source) | ||
| : entry.source; | ||
| if (!source) { |
There was a problem hiding this comment.
Nit: toRelativeModuleSpecifier always returns a string (it either prepends ./ or returns the relative path as-is from path.relative), so !source is always false here. This guard is dead code.
Not blocking — it's harmless as a defensive check, but it could confuse readers into thinking toRelativeModuleSpecifier can return a falsy value.
| if (inProgress.has(filePath)) { | ||
| return cache.get(filePath) ?? new Map(); | ||
| } |
There was a problem hiding this comment.
Minor: The inProgress guard on line 494 checks cache.get(filePath) as a fallback, but cache.get(filePath) was already checked on line 491-492 and would have returned early. So this fallback always evaluates to new Map(). The intent is correct (prevent re-entrant calls from seeing a partially-built map), but the cache.get(filePath) ?? part is dead code since the cache lookup on line 491 already handles that case.
|
Review submitted. Here's a summary of what I posted: Review: REQUEST_CHANGES on PR #847 Blocking issue:
Non-blocking comments (4 inline):
|
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
|
Follow-up fix for the remaining CI blocker:
This keeps the test aligned with the intended Next.js-style behavior we discussed:
Validation rerun:
Result:
|
|
/bigbonk review |
There was a problem hiding this comment.
Review: fix(plugins): resolve optimized imports past local barrels
The core fix is well-designed: recursively resolving past intermediate local barrels to the concrete leaf module, and flattening export * boundaries before @vitejs/plugin-rsc inspects them, correctly addresses the issue-845 failure mode. The iterative review cycle addressed the critical issues (JSX parsing regression, phantom exports, reentry guard, client fixture). CI unit tests are now green.
The test split into focused files is a welcome improvement for maintainability.
A few items I'd flag:
Source map regression (actionable)
The map: null change replaces s.generateMap({ hires: "boundary" }) without explanation. Every other MagicString-based transform in this codebase returns proper source maps (fonts.ts, instrumentation-client.ts, index.ts). This degrades dev-mode stack traces for files with rewritten barrel imports. The impact is limited since changes are confined to import lines, but it's an unnecessary regression from the prior behavior.
Dead code (non-blocking)
toRelativeModuleSpecifieralways returns a string, so the!sourceguard at line 977 is unreachable.- The
cache.get(filePath) ??fallback in theinProgressguard (line 495) is dead — the cache lookup at line 491-492 already returns early.
Both are harmless but slightly misleading to future readers.
Design observations (non-blocking, for awareness)
- The wildcard merge path (line 669) doesn't apply
resolveConcreteLocalEntryto merged entries, unlike theExportNamedDeclarationpath. This works for the antd shape but could miss deeper barrel chains via wildcard. The author acknowledged this as a follow-up. buildSafeWildcardExportMapreturnsnull(bail) on circular references whilebuildExportMapFromFilereturns an empty map (non-fatal). The inconsistency is safe but worth documenting if this area gets more complex.
Overall this is in good shape. The fix is well-targeted, the test coverage exercises the real failure mode, and the iterative review addressed all critical items.
| return { | ||
| code: s.toString(), | ||
| map: s.generateMap({ hires: "boundary" }), | ||
| map: null, |
There was a problem hiding this comment.
This drops source maps without explanation. Every other MagicString transform in this codebase (fonts.ts, instrumentation-client.ts, index.ts) returns s.generateMap({ hires: "boundary" }). The impact is small (only import lines change), but it's an unnecessary regression from the prior behavior and breaks Vite's source map composition chain for these files.
| map: null, | |
| map: s.generateMap({ hires: "boundary" }), |
| const source = entry.source.startsWith("/") | ||
| ? toRelativeModuleSpecifier(normalizedId, entry.source) | ||
| : entry.source; | ||
| if (!source) { |
There was a problem hiding this comment.
Dead code: toRelativeModuleSpecifier always returns a string (it either prepends ./ or returns the path.relative result as-is), so !source is always false. This guard can never trigger.
Not blocking — harmless as a defensive check, but could confuse readers into thinking toRelativeModuleSpecifier can return a falsy value.
| if (cached) return cached; | ||
|
|
||
| if (inProgress.has(filePath)) { | ||
| return cache.get(filePath) ?? new Map(); |
There was a problem hiding this comment.
Minor: the cache.get(filePath) ?? fallback here is dead code. If cache.get(filePath) returned a truthy value, the early return on line 491-492 would have already fired. At this point cache.get(filePath) is always undefined, so this always evaluates to new Map().
The intent (prevent re-entrant calls from seeing a partially-built map) is correct — just the fallback expression is misleading.
| import vinext from "../packages/vinext/src/index.js"; | ||
|
|
||
| async function withTempDir<T>(prefix: string, run: (tmpDir: string) => Promise<T>): Promise<T> { | ||
| const tempParent = path.resolve(import.meta.dirname, "../.sisyphus/tmp"); |
There was a problem hiding this comment.
Minor: the temp directory is placed under .sisyphus/tmp (relative to the test file's directory). Is .sisyphus/ gitignored? If not, this could leave artifacts in the working tree on test interruption (even though the finally block cleans up normally). Other tests in this PR use os.tmpdir() which is a safer default for temp files.
|
The review was submitted successfully. Here's a summary of what was posted: Review: COMMENT on PR #847 The review finds the PR in good shape overall. The core fix — recursively resolving past intermediate local barrels to the concrete leaf module and flattening 4 inline comments posted:
Top-level observations:
|
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
|
Follow-up for the last four inline comments:
Validation rerun:
Result:
|
|
/bigbonk review |
There was a problem hiding this comment.
Review: fix(plugins): resolve optimized imports past local barrels
The core fix is well-designed and correctly targeted at issue-845. The approach of recursively resolving past intermediate local barrels to the concrete leaf module, and flattening export * boundaries before @vitejs/plugin-rsc inspects them, is sound. The iterative review cycle addressed all critical issues (JSX parsing, phantom exports, reentry guard, client fixture, source maps). CI is green on the unit and Check jobs, with remaining integration jobs still pending at review time.
The test split into focused files is a welcome improvement for maintainability.
I have a few items to flag — one functional concern and several minor/non-blocking items.
Functional: canRewrite is always true
The canRewrite variable at line 971 is initialized to true and never set to false anywhere in the loop body. The guard if (!canRewrite || bySource.size === 0) continue on line 986 therefore only ever triggers on bySource.size === 0. This looks like leftover scaffolding from an earlier iteration where there was a bail condition. It's not a bug (it's equivalent to just checking bySource.size === 0), but it's misleading — it implies there's a code path that sets canRewrite = false when there isn't one.
Non-blocking items
-
buildFallbackExportMapdoesn't handleexport default function()(anonymous) — The regex/^\s*export\s+default\s+([A-Za-z_$][\w$]*)\s*;?/gmrequires a named identifier afterexport default. Anonymousexport default function() {}won't match. This is fine as a conservative fallback, but worth noting since it means the fallback path silently drops anonymous default exports. The AST-based path handles them correctly, so this only matters for.jsfiles containing JSX that fail to parse. -
Build test
withTempDirsymlinksnode_modulesbefore writing fixturenode_modules/antd— Inoptimize-imports-build.test.ts,withTempDirsymlinks the repo'snode_modulesinto the temp dir on line 11-15, thenwriteFixtureFilewrites files undernode_modules/antd/on lines 130-168. Because the symlink makesnode_modulespoint to the repo's realnode_modules, thewriteFixtureFilecalls that write intonode_modules/antd/are writing into the actual repo'snode_modulesdirectory. This works ifantdisn't installed in the repo (the directory is created fresh), but thefinallyblock only removes the temp dir — it doesn't clean up files written through the symlink into the realnode_modules. If the test is interrupted or the cleanup logic changes, staleantdfiles could be left in the workspace'snode_modules. -
Acknowledged follow-ups — The wildcard-merge
resolveConcreteLocalEntrygap and the dual-traversal inbuildSafeWildcardExportMapwere correctly deferred as non-blocking by the author. Agree with that assessment.
| group.exports.push({ exported, originalName: entry.originalName }); | ||
| } | ||
|
|
||
| if (!canRewrite || bySource.size === 0) continue; |
There was a problem hiding this comment.
canRewrite is initialized to true on line 971 and never set to false anywhere in the loop body (lines 972-984). This guard is equivalent to just if (bySource.size === 0) continue.
Looks like leftover scaffolding from an earlier iteration. Either remove canRewrite entirely, or add a comment indicating what condition would set it to false if you plan to add one later.
| if (!canRewrite || bySource.size === 0) continue; | |
| if (bySource.size === 0) continue; |
| async function withTempDir<T>(prefix: string, run: (tmpDir: string) => Promise<T>): Promise<T> { | ||
| const tmpDir = await mkdtemp(path.join(os.tmpdir(), prefix)); | ||
| fs.symlinkSync( | ||
| path.resolve(import.meta.dirname, "../node_modules"), | ||
| path.join(tmpDir, "node_modules"), | ||
| "junction", | ||
| ); |
There was a problem hiding this comment.
This symlinks the repo's real node_modules into the temp dir. The test then calls writeFixtureFile to write into node_modules/antd/ (lines 130-168), which follows the symlink and writes into the actual repo's node_modules directory.
This works as long as antd isn't already installed in the workspace. But if the test is interrupted before the finally cleanup, or if antd ever gets added as a dev dependency, you'll have stale fixture files in the real node_modules.
Consider writing the antd fixture before the symlink and excluding it from the symlink target, or creating node_modules/antd as its own directory (not through the symlink) by removing the symlink for just that path. Alternatively, a brief comment explaining the constraint would help future maintainers.
| } | ||
| } | ||
|
|
||
| for (const match of content.matchAll(/^\s*export\s+default\s+([A-Za-z_$][\w$]*)\s*;?/gm)) { |
There was a problem hiding this comment.
Minor: this regex only matches export default <identifier>, not export default function() {} (anonymous) or export default class {}. The AST path handles these correctly, so this only affects the fallback when .js files containing JSX fail to parse. The conservative behavior (silently dropping the anonymous default) is fine, but a brief comment noting this limitation would help readers understand the intentional gap.
Also: export default function Foo() {} will match "default" via recordDirectExport("default", "default") on line 198, but Foo is captured by the separate export function pattern on line 187 since the line also starts with export. This means Foo ends up in the map twice — once from line 187's declarationPatterns match and once potentially from line 200-202 (though the !exportMap.has(ident) guard on line 200 prevents the second write). This is correct but the interaction is subtle.
| inProgress, | ||
| ); | ||
| const nextEntry = subMap.get(exportName); | ||
| if (!nextEntry) return entry; |
There was a problem hiding this comment.
When buildExportMapFromFile returns a map for candidate but the target exportName isn't in it, this returns the original entry without trying subsequent candidates from localModuleCandidates.
This means if ./button resolves to button.js (exists, but doesn't export Button) while button/index.js (also exists, does export Button) is a later candidate, we'd stop at the wrong file. This is an edge case unlikely to appear in real packages, but worth being aware of — the break-on-first-readable-file semantic in the wildcard resolution loop (line 674) has the same behavior, so at least it's consistent.
|
Review submitted successfully. Here's a summary of what I posted: Review: COMMENT on PR #847 The core fix is well-designed — recursively resolving past intermediate local barrels and flattening 4 inline comments posted:
|
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
|
Follow-up for the latest review comments:
That keeps the reviewer-requested temp-dir cleanup while avoiding writes into the real workspace Validation rerun:
Result:
|
|
One note on the two remaining non-blocking observations that I did not fold into this PR:
For this PR I tried to keep the scope tightly focused on:
|
|
/bigbonk review |
There was a problem hiding this comment.
Review: fix(plugins): resolve optimized imports past local barrels
This is a well-executed fix. The core approach — recursively resolving past intermediate local barrels to the concrete leaf module, and flattening export * boundaries before @vitejs/plugin-rsc inspects them — is sound and correctly targeted to the issue-845 failure mode.
The iterative review cycle addressed all critical issues:
- JSX parsing regression from
enforce: "pre"→parseAstWithFileLang - Phantom
"function"/"class"exports in fallback path - Reentry guard for circular barrel analysis
- Client environment fixture isolation
- Source map restoration
- Dead code cleanup (
canRewrite,!sourceguard,cache.getfallback) - Build test temp directory isolation
The test split into focused files is a welcome improvement for maintainability. The new helpers (localModuleCandidates, toRelativeModuleSpecifier, findOptimizedPackageForFile, resolveLocalModuleFile, buildFallbackExportMap, buildSafeWildcardExportMap, resolveConcreteLocalEntry) are well-factored and appropriately scoped.
CI: Check and Vitest (unit) are green. Several E2E and integration jobs are still pending at review time but the passing unit and check jobs cover the changed code paths.
I have a few non-blocking observations below — none of them are merge blockers.
| } | ||
| } | ||
|
|
||
| for (const match of content.matchAll(/^\s*export\s+default\s+([A-Za-z_$][\w$]*)\s*;?/gm)) { |
There was a problem hiding this comment.
Non-blocking observation: export default function() {} (anonymous default function) won't match this regex since it requires a named identifier. The AST-based path handles this correctly, so it only matters for .js files containing JSX that fail AST parsing and hit the fallback.
Also worth noting the subtle interaction: export default function Button() {} matches both this regex (capturing "Button" after filtering "function") and the declarationPatterns function regex on line 187 (which captures "Button" from the function declaration). The !exportMap.has(ident) guard on line 200 prevents the double-write, which is correct.
No action needed — just documenting for future readers.
| if (!nestedMap) return null; | ||
| } | ||
|
|
||
| return buildExportMapFromFile(filePath, readFile, cache, new Set<string>(), content); |
There was a problem hiding this comment.
Non-blocking: the validation pass (lines 228–245) and the building pass (this buildExportMapFromFile call) use separate visited sets, meaning files can be visited twice. The exportMapCache mitigates re-parsing cost, so the duplicate traversal is a performance nit rather than a correctness issue. Worth noting if this area gets more complex — the two passes could potentially share results.
| candidateContent, | ||
| inProgress, | ||
| ); | ||
| for (const [name, entry] of subMap) { |
There was a problem hiding this comment.
Previously flagged and acknowledged by author: the wildcard merge path here copies entries from subMap without applying resolveConcreteLocalEntry. This means if a wildcard sub-module itself re-exports through another intermediate barrel (export * from "./group" where group/index.js has export { Foo } from "./foo/index.js"), the entries won't be chased to the leaf.
This works for the antd shape (root barrel uses named re-exports), and the author correctly scoped this as a follow-up. Just keeping the note visible for the merge reviewer.
| inProgress, | ||
| ); | ||
| const nextEntry = subMap.get(exportName); | ||
| if (!nextEntry) return entry; |
There was a problem hiding this comment.
Minor: when the first readable candidate file has a valid export map but doesn't contain exportName, we return the original entry without trying later candidates. This is the break-on-first-readable-file semantic, consistent with the wildcard resolution loop at line 674. Unlikely to matter in practice — in real packages, the first readable file at a given path is the right one — but worth being aware of for any edge case where ./button.js exists but doesn't export Button while ./button/index.js does.
| fs.writeFileSync(absPath, content); | ||
| } | ||
|
|
||
| function readTextFilesRecursive(root: string): string { |
There was a problem hiding this comment.
Nit: readTextFilesRecursive follows symlinks (the react/react-dom symlinks created by symlinkWorkspacePackage) and would recursively read all .js files under the real react package if they end up in the dist/ tree. In practice this is fine because the build output won't contain symlinks to workspace packages in the output dir, but if the build ever emits a symlink (e.g., Vite preserves symlinks in some configurations), this could produce unexpectedly large buildOutput strings and slow assertions.
Not blocking — the current build test works correctly.
|
Review submitted successfully. Here's a summary of what I posted: Review: APPROVE on PR #847 The PR is in good shape after 6 rounds of iterative review that addressed all critical issues. The core fix — recursively resolving past intermediate local barrels to the concrete leaf module and flattening All prior critical issues were resolved:
5 non-blocking inline comments for awareness/documentation:
|
2 participants
Summary
optimize-importsso optimized package imports keep following local re-export chains to the concrete leaf module instead of stopping at an intermediate barrel likeantd/es/button/index.jsexport *boundaries inside optimized packages before downstream graph analysis so@vitejs/plugin-rscno longer sees the issue-845 client-boundary shapeantdrepro pathValidation
./node_modules/.bin/vp test run tests/optimize-imports.test.ts -t "multi-hop antd barrel"./node_modules/.bin/vp test run tests/optimize-imports-build.test.tsIssue
Fixes #845.
Current solution
The bug happened because
vinext:optimize-importsused to stop at an intermediate local barrel (antd/es/button/index.js). That file is a"use client"boundary withexport *, so@vitejs/plugin-rsclater failed withunsupported ExportAllDeclaration.The fix keeps the rewrite generic rather than
antd-specific:.jsleaf fails, so the local-barrel walk can still reach the leaf moduleThis means the app import now rewrites to the concrete leaf module instead of leaking the intermediate
export *barrel into the RSC build.