fix(rsc): resolve export * from in use client modules#1373
Conversation
`@vitejs/plugin-rsc`'s `rsc:use-client` transform throws
"unsupported ExportAllDeclaration" when a `"use client"` module contains
a `export * from '...'` or `export * as Ns from '...'` declaration. This
broke any app that pattern-matched Next.js's `test/e2e/app-dir/rsc-basic`
fixture, which puts `export *` re-exports inside client boundaries.
Add a pre-pass Vite plugin (`vinext:use-client-export-all`) that, in the
RSC environment, rewrites `export * from '...'` to an explicit
`export { a, b, c } from '...'` form by resolving the target module via
the Rollup plugin context and enumerating its named exports (recursing
through chained `export *` re-exports). `export * as Ns from '...'` is
rewritten to `import * as Ns from '...'; export { Ns };`. The default
export is intentionally not forwarded, matching standard ESM semantics.
The plugin is registered before the auto-injected `@vitejs/plugin-rsc`
in vinext's plugin array, at the default enforce position so it runs
after Vite's built-in TS/JSX transform.
Add `tests/use-client-export-all-build.test.ts` with two production
build fixtures (one for `export *` and one for `export * as Ns`),
both ported from
`.nextjs-ref/test/e2e/app-dir/rsc-basic/components/export-all/`.
Closes #1352
commit: |
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
/bigbonk review |
Replace the multi-alternation backtracking regex used by `quickHasUseClient` with a single-pass manual scanner. The previous pattern (a `(?:\/\*...\*\/|//...)*` group followed by a string literal) could exhibit catastrophic backtracking on inputs starting with `/*` and many `*//*` repetitions — flagged by CodeQL as a high-severity ReDoS (js/redos) on the PR diff.
|
/bigbonk |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
Clean, well-structured workaround for the upstream @vitejs/plugin-rsc limitation. The approach is sound — a pre-pass transform at the same enforce level but earlier in the array is the right strategy, and the recursive export collection with cycle detection is correct. Two issues to address below.
Blocking
HAS_USE_CLIENT_RE has catastrophic backtracking (confirmed)
CodeQL flagged this correctly. The regex on line 34:
const HAS_USE_CLIENT_RE = /^\s*(?:\/\*[\s\S]*?\*\/\s*|\/\/[^\n]*\n\s*)*['"]use client['"]/;I confirmed this hangs on inputs like "/*" + "*//*".repeat(50) + "x" (>10s, effectively infinite). The [\s\S]*? inside /* ... */ can match */ characters, creating ambiguity with the outer repetition (...)*. This is a real concern since every .js/.ts file in the project runs through quickHasUseClient in the RSC environment.
This repo already has a battle-tested O(n) manual parser for exactly this: hasReactDirective() in index.ts:443. That function handles BOM, hashbang, line comments, block comments, and directive prologues with zero backtracking risk, and it already checks for "use client".
Suggested fix: Extract hasReactDirective() to a shared utility and reuse it here. It does exactly what quickHasUseClient needs. Alternatively, if you want to keep a regex, the simplest fix is a two-step approach: code.includes("use client") && <regex> where the regex doesn't need to handle comments at all (just check the directive is at the start of a line), but honestly the existing function is the cleanest path.
Non-blocking observations
1. collectNamedExports misses destructured exports
collectNamedExports only handles d.id.type === "Identifier" for VariableDeclaration declarators (line 95). Destructured exports like export const { Provider, Consumer } = createContext() or export const [first, second] = tuple produce ObjectPattern / ArrayPattern nodes, which are silently skipped. The generated export { ... } from will be missing those names.
In practice this is low-risk — destructured re-exports in "use client" barrel files are rare, and the fallback behavior (missing names → upstream error → user switches to explicit re-exports) is reasonable. But worth noting for completeness. If you want to handle it, recursing into the pattern's identifiers is straightforward.
2. this.load() returns transformed code — this is correct
I verified that this.load({ id }) returns fully transformed code (post-oxc TS/JSX transform), so parseAst in collectNamedExports receiving standard ESM is safe. The comment on line 213-216 about parseAst not accepting TS/JSX is accurate for the top-level findExportAllDeclarations call (which operates on the code parameter passed to transform), and the recursive collectNamedExports path gets pre-transformed code from this.load(). Both paths are correctly handled.
3. Plugin ordering is correct
Confirmed: createUseClientExportAllPlugin() is registered at line 2277 in the plugins array, while rscPluginPromise (containing rsc:use-client) is pushed at line 4185. Both are at default enforce. Transform hooks fire in array order at the same enforce level, so this plugin's rewrite runs before the upstream transform sees the module.
4. Test coverage is good
The two test cases (chained export * with nested re-exports, and export * as Namespace) cover the main code paths well. The fixture structure mirrors the Next.js rsc-basic/components/export-all pattern as claimed.
Overall this is a well-motivated and well-documented workaround. Fix the regex and it's ready to merge.
| namespaceName: string | null; | ||
| }; | ||
|
|
||
| const HAS_USE_CLIENT_RE = /^\s*(?:\/\*[\s\S]*?\*\/\s*|\/\/[^\n]*\n\s*)*['"]use client['"]/; |
There was a problem hiding this comment.
Confirmed: this regex causes catastrophic backtracking. The pattern [\s\S]*? inside /* ... */ can match */ itself, creating exponential ambiguity with the outer (...)* repetition.
Repro:
const HAS_USE_CLIENT_RE = /^\s*(?:\/\*[\s\S]*?\*\/\s*|\/\/[^\n]*\n\s*)*['\"'"]use client['\"']/;
// Hangs for >10s:
HAS_USE_CLIENT_RE.test("/*" + "*//*".repeat(50) + "x");hasReactDirective() in index.ts:443 already does this correctly — manual O(n) comment-skipping parser that handles BOM, hashbang, line comments, block comments, and directive prologues. It already checks for "use client". Extract it to a shared module and reuse it here.
|
Review posted successfully. Here's a summary of what I found: Review Summary for PR #1373Verdict: Request Changes — one blocking issue, the rest looks good. Blocking: Catastrophic regex backtracking (line 34)The Fix: Reuse the existing Non-blocking observations
|
`collectNamedExports` previously only recognised `Identifier` declarator ids,
silently dropping destructured exports like `export const { Provider, Consumer } = ...`
or `export const [a, b] = ...`. The rewritten `export { ... } from` clause
would omit those names and consumers would see `undefined` at runtime.
Walk binding patterns (ObjectPattern / ArrayPattern / RestElement /
AssignmentPattern) and collect every bound identifier. Add a regression test
that builds a `"use client"` barrel re-exporting destructured bindings.
|
Thanks @ask-bonk — addressed your review:
On the /bigbonk |
There was a problem hiding this comment.
Follow-up Review
Both issues from the initial review are resolved. Approving.
Blocking (ReDoS) — fixed in 36776a9
The HAS_USE_CLIENT_RE regex is gone, replaced by quickHasUseClient() — a manual single-pass scanner that visits each character at most once. The logic is correct:
- Skips whitespace (space, tab, LF, CR, FF, VT)
- Skips
// line commentsto end-of-line - Skips
/* block comments */by scanning for*/without backtracking - At the first non-whitespace/non-comment character, checks for a quote followed by
use clientfollowed by the matching quote
O(n) worst case, no backtracking possible. CodeQL confirms it's clean.
On the hasReactDirective() reuse question: the explanation is reasonable. The two functions have different semantics (hasReactDirective walks past "use strict" and checks for both "use client" and "use server"; quickHasUseClient is a "use client"-only fast filter that doesn't walk past prior directives). Merging them would either widen behavior or over-parameterize. Fine to leave as a follow-up if it ever becomes worth it.
Non-blocking #1 (destructured exports) — fixed in 53343b3
collectPatternNames correctly walks all ESTree binding pattern types:
Identifier→ adds the name directlyObjectPattern→ iteratesproperties, handlingRestElement(walks.argument) and regular properties (walks.value, which is the binding pattern for the property)ArrayPattern→ iterateselements, recursing into eachRestElement→ recurses into.argumentAssignmentPattern→ recurses into.left(the binding target, ignoring the default value)
This covers the full ESTree BindingPattern grammar. The regression test exercises both export const { First, Second } = ... (ObjectPattern) and export const [Third] = ... (ArrayPattern) through a "use client" barrel file with export * from "./inner".
CI
CodeQL, Check, and most E2E/deploy jobs are green. Vitest unit/integration and a couple E2E jobs are still running but the relevant test file (use-client-export-all-build.test.ts) is included in the integration shard, and Check already passed (format + lint + types).
|
Approved PR #1373. Both issues from the initial review are resolved:
The |
There was a problem hiding this comment.
Both fixes verified, LGTM.
ReDoS (36776a9): quickHasUseClient is a clean O(n) manual scanner — no regex, no backtracking, each character visited at most once. CodeQL green.
Destructured exports (53343b3): collectPatternNames correctly walks the full ESTree BindingPattern grammar (Identifier, ObjectPattern, ArrayPattern, RestElement, AssignmentPattern). Regression test covers both export const { First, Second } = ... and export const [Third] = ... through a "use client" barrel.
On the hasReactDirective reuse question: agree with keeping them separate. The two have different semantics (hasReactDirective walks past "use strict", checks both "use client" and "use server"; quickHasUseClient is a "use client"-only fast filter). Merging would over-parameterize for no real benefit. Not worth a follow-up unless the pattern proliferates further.
CI: Check, CodeQL, Vitest (unit), most E2E and deploys all green. Three integration/E2E jobs still pending — nothing suggests they'll fail given the existing passes.
|
Review posted. Both fixes are solid:
CI is nearly all green, PR is approved and ready to merge once the remaining 3 pending jobs finish. |
Ports the build fixture from #1373 to verify the `@vitejs/plugin-rsc` 0.5.27 bump resolves the upstream "unsupported ExportAllDeclaration" error. Covers bare `export *`, nested/renamed re-exports, `export * as Ns from`, and destructured re-exported bindings. The version bump fixes this natively, so the vinext-side workaround plugin from #1373 is no longer needed. Refs #1352
* chore(deps): bump `@vitejs/plugin-rsc` to 0.5.27 * test(rsc): cover `export * from` in 'use client' modules (#1352) Ports the build fixture from #1373 to verify the `@vitejs/plugin-rsc` 0.5.27 bump resolves the upstream "unsupported ExportAllDeclaration" error. Covers bare `export *`, nested/renamed re-exports, `export * as Ns from`, and destructured re-exported bindings. The version bump fixes this natively, so the vinext-side workaround plugin from #1373 is no longer needed. Refs #1352
2 participants
Summary
@vitejs/plugin-rsc'srsc:use-clienttransform throwsunsupported ExportAllDeclarationwhenever a"use client"module containsexport * from '...'(orexport * as Ns from '...'). This breaks any app that follows Next.js'stest/e2e/app-dir/rsc-basicpattern of puttingexport *re-exports inside client boundaries.Adds a new pre-pass Vite plugin,
vinext:use-client-export-all, that runs in the RSC environment beforersc:use-clientand rewrites:export * from '...'→export { a, b, c, ... } from '...'(named exports enumerated by resolving and parsing the target module; nestedexport *chains are recursed;defaultis intentionally not forwarded, matching ESM)export * as Ns from '...'→import * as Ns from '...'; export { Ns };(a single named export the upstream transform can wrap as a client-reference proxy)The plugin is registered before the auto-injected
@vitejs/plugin-rscin vinext's plugin array, at the default enforce position so it runs after Vite's built-in TS/JSX transform (parseAst doesn't accept TS/JSX).Design notes
export *differently — it sets the module'sassumedSourceTypeback tomoduleso webpack handles the re-export natively (seenext-flight-loader/index.ts). We can't do the same in Rolldown/Vite, so we pre-expand the declaration instead. The visible behavior matches: each re-exported named export becomes its own client-reference proxy..nextjs-ref/test/e2e/app-dir/rsc-basic/components/export-all/*(chainedexport *, renamed re-export, and a namespace re-export case).node_modules/@vitejs/plugin-rsc/dist/transforms/index.js:338—if (!options.ignoreExportAllDeclaration && node.type === "ExportAllDeclaration") throw new Error("unsupported ExportAllDeclaration");. The plugin exposes no opt-out, so a pre-pass is the only workaround without patchingnode_modules.Upstream tracking
use-clienttransform throws onexport * from(ExportAllDeclaration) vitejs/vite-plugin-react#1233 — filed for this PR. Even if upstream lands a fix, our pre-pass is a safe no-op on already-supported code paths.Test plan
pnpm test tests/use-client-export-all-build.test.ts— both new cases pass; both fail onmainwith the expected errorpnpm test tests/app-router.test.ts tests/build-optimization.test.ts tests/standalone-build.test.ts— 402 existing tests still passpnpm run check— clean (format, lint, types)Closes #1352