fix: use loader for client reference#1141
Conversation
commit: |
|
/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: use loader for client reference
Good diagnosis in the PR description — the Safari crash from undefined module exports during RSC client reference resolution is a real issue, and the grouping-by-virtual-RSC-facade explanation is convincing. The fix approach (intercepting the \0virtual:vite-rsc/client-references module via transform to emit per-module direct imports instead of the RSC plugin's default grouped-chunk approach) is sound.
Some issues and questions below.
Core plugin
The plugin itself is clean. The transform hook at enforce: "post" correctly intercepts after the RSC plugin's load hook has produced the default grouped output. The groupChunkId = id mutation to keep the assets manifest correct is the right idea.
However, there's an issue with how groupChunkId is set. See inline comment.
Test concerns
The test is ambitious — it scaffolds an entire fixture app in a temp directory, runs a full production build, and serves it. This is good for confidence but has some robustness and CI cost concerns. See inline comments.
CI changes
Adding WebKit to CI is the right call given the bug is Safari-specific. The cache key branching and conditional install logic look correct. One concern about the dev server vs production build mismatch.
Overall: The core fix looks correct and well-motivated. The main issues are around the test structure and a potential correctness gap in groupChunkId assignment. Happy to re-review after addressing the inline comments.
|
|
||
| for (const [id, meta] of metaEntries) { | ||
| // The RSC assets manifest reads this field to collect JS/CSS deps. | ||
| meta.groupChunkId = id; |
There was a problem hiding this comment.
The RSC plugin's assets manifest builder does assetDeps[meta.groupChunkId] to find each client reference's JS/CSS dependencies. The default RSC plugin sets groupChunkId to a virtual group module ID (\0virtual:vite-rsc/client-references/group/<name>) which is a key in the output bundle.
Here you set groupChunkId = id where id is the key from clientReferenceMetaMap — which is the resolved module ID of the original source file (e.g., /abs/path/to/Button.tsx). But in the generated client-references module, each loader does import(importId) (possibly via the resolved-id proxy), not import(id).
For the common case where id === meta.importId, these match. But ClientReferenceMeta has importId as a separate field specifically because they can differ (e.g., package sources where packageSource is set). If id !== meta.importId, then assetDeps[meta.groupChunkId] would look up the wrong key and the assets manifest would have missing JS/CSS deps for that reference.
Should this be:
| meta.groupChunkId = id; | |
| meta.groupChunkId = meta.importId; |
Or even better, should groupChunkId be the withResolvedIdProxy(meta.importId) value (matching what actually gets import()-ed in the generated code), since that's the module ID the bundler will create a chunk for?
Either way, can you verify that the assets manifest is correct for a build with package-sourced client components (e.g., a "use client" component from node_modules)?
| | undefined | ||
| )?.api; | ||
| }, | ||
| transform(_code, id) { |
There was a problem hiding this comment.
Using transform rather than load is an unusual choice here. The RSC plugin generates this module via a load hook. By intercepting in transform (with enforce: "post"), you're replacing the already-loaded code, which works but means the RSC plugin's load hook still runs and populates clientReferenceGroups and sets groupChunkId on each meta — and then your transform immediately overwrites groupChunkId.
This isn't necessarily wrong, but it creates a hidden ordering dependency: the RSC plugin's load must run first (to populate renderedExports and serverChunk on the metas), and then this transform overwrites the grouping. A comment explaining this would help future maintainers.
Alternatively, could this be done via the RSC plugin's clientChunks option instead? That's the intended extension point for customizing client reference grouping. It would avoid the fragile coupling to the RSC plugin's internal virtual module ID and generation logic.
| const exports = meta.renderedExports | ||
| .slice() | ||
| .sort() | ||
| .map((name) => ` get ${JSON.stringify(name)}() { return m[${JSON.stringify(name)}]; },`) |
There was a problem hiding this comment.
Nit: the lazy getter pattern is clever for avoiding eager evaluation, but each property access will re-index into m every time. In practice this is fine (React only reads each export once during hydration), but it's a subtlety worth a brief comment — especially since the default RSC plugin generates direct property copies rather than getters.
| await fs.writeFile( | ||
| path.join(clientRefsDir, `probe-${index}.tsx`), | ||
| `"use client"; | ||
| await Promise.resolve(); |
There was a problem hiding this comment.
This await Promise.resolve() is the key ingredient that forces async module evaluation and exposes the Safari timing bug. Could you add a comment explaining why this is needed? Something like:
| await Promise.resolve(); | |
| // Top-level await forces async module evaluation, which exposes the | |
| // Safari/WebKit timing bug where client reference module namespaces | |
| // can be observed as undefined before initialization completes. | |
| await Promise.resolve(); |
(The suggestion is for the generated code template, so it would go in the string literal above.)
| configFile, | ||
| logLevel: "silent", | ||
| }); | ||
| await builder.buildApp(); |
There was a problem hiding this comment.
This does a full createBuilder + buildApp() inside a Playwright fixture. That's a heavyweight operation that adds significant time to each CI run of the browser-specific projects. A few concerns:
-
Timeout risk. The default Playwright test timeout is 30s. A production build of even a small app with RSC can take 10-20s depending on the runner. If the build is slow, the test will flake. Consider either increasing the test timeout or (better) adding a
test.setTimeout()call. -
Build caching. Every test run rebuilds from scratch in a fresh tmpdir. If more browser-specific tests are added later, this could become expensive. Consider building once in a
globalSetupand sharing the output across tests. -
Import of
viteat test time. Theawait import("vite")inside the fixture means Vite's module graph is loaded in the Playwright worker process. This is unusual and could cause issues if Vite's process-level state conflicts with anything else.
| const { startProdServer } = await import( | ||
| pathToFileURL(path.resolve(process.cwd(), "packages/vinext/dist/server/prod-server.js")).href | ||
| ); | ||
| const started = await startProdServer({ | ||
| host: "127.0.0.1", | ||
| port: 0, | ||
| outDir: path.join(fixtureRoot, "dist"), | ||
| noCompression: true, | ||
| }); |
There was a problem hiding this comment.
The test imports prod-server.js from packages/vinext/dist/. This means the vinext package must be built before this test runs. The CI workflow does run vp run build before E2E tests, so this works in CI. But for local development (reuseExistingServer is true when !process.env.CI), a developer who hasn't built the package will get a confusing error.
The other browser-specific projects share the app-router dev server which starts via the Playwright webServer config. But this test bypasses that entirely and builds+serves its own app. That's a mismatch worth documenting — the baseURL from the project config (http://localhost:4174) is never used by this test.
Consider adding a comment at the top of the file explaining that this test manages its own build+server lifecycle and doesn't use the shared dev server.
|
|
||
| const appRouterBrowserSpecificTests = "**/app-router/**/*.browser.spec.ts"; | ||
| const appRouterServer = { | ||
| command: "npx vp dev --port 4174", |
There was a problem hiding this comment.
The appRouterServer starts a dev server (vp dev), but the test in client-reference-runtime-map.browser.spec.ts does its own production build and doesn't use this server at all. This means for the app-router-chrome-browser-specific and app-router-webkit-browser-specific CI jobs, the dev server is started but never hit.
That's wasted CI time (waiting for the dev server to be ready) and potentially confusing. Would it be better to have the browser-specific projects use a different (or no) server config? Or is the intent that future *.browser.spec.ts tests will use the dev server, and this particular test is the exception?
|
|
||
| - name: Install Playwright WebKit with system dependencies | ||
| if: ${{ matrix.project == 'app-router-webkit-browser-specific' }} | ||
| run: vp exec playwright install --with-deps webkit |
There was a problem hiding this comment.
WebKit's system dependencies can be large and flaky to install on CI. --with-deps installs OS-level packages (libwoff1, libvpx, etc.) via apt. A few things to watch:
-
The cache key is
playwright-webkit-...which caches the browser binary, but the system deps are installed via apt and not cached. This means every CI run of the webkit job will re-install system deps. Consider whether this adds meaningful time. -
If the webkit test is the only test that exercises this fix (since the bug is Safari-specific), and it fails for infrastructure reasons (apt failures, WebKit install issues), it could block PRs. Is this job in the required checks? If so, consider making it non-required initially until it proves stable.
| const manager = rscApi?.manager; | ||
| if (!manager || manager.isScanBuild) return null; | ||
|
|
||
| const metaEntries = Object.entries(manager.clientReferenceMetaMap ?? {}).filter( |
There was a problem hiding this comment.
Minor: the ?? {} fallback on clientReferenceMetaMap is unnecessary if the RSC plugin always initializes this as an empty object (which it does in the RscPluginManager class). Not a bug, but the defensive fallback could mask issues where rscApi is retrieved from the wrong plugin.
|
Review posted successfully. Here's a summary of the feedback: Core fix — The approach of intercepting Key inline comments:
|
|
resolved |
|
/bigbonk review |
There was a problem hiding this comment.
Re-review: fix: use loader for client reference
The tweaks commit addresses the feedback from the first review well:
-
groupChunkId = idcorrectness: The added comment explains why the resolved map key is correct (it matcheschunk.moduleIdsin the output bundle, whereasmeta.importIdcan be a bare specifier for package-sourced references). The new@vinext-test/client-packagefixture with CSS and the build-timeassertPackageClientReferenceCssDepsassertion directly validate that the assets manifest is correct for the package-source case. This was the main concern — now covered both by explanation and by test. -
Wasted dev server: Browser-specific projects now use
server: nulland thewebServerarray filters nulls. No unnecessary dev server startup in CI for these projects. -
Comments: Lazy getter rationale,
await Promise.resolve()explanation, transform ordering explanation, and the top-level test file comment are all added. -
Removed
?? {}: The unnecessary defensive fallback onclientReferenceMetaMapis gone. -
linkFixtureNodeModulesrefactored: Per-entry symlinking with.vite-tempexclusion supports injecting the fake@vinext-test/client-packagewithout conflicting with Vite's temp directory.
All CI checks pass including both new browser-specific projects (chrome: 50s, webkit: 3m). The webkit job timing is reasonable.
Two minor observations (not blocking):
-
The
findCssAssetContaininghelper uses exception-based control flow (throw new Error("CSS asset not found")) for a normal "not found" case. Anullreturn would be more idiomatic, but it works fine and is test-only code. -
The local standalone Playwright config (
playwright.nextjs-compat.config.ts) adds browser-specific projects but doesn't configure atimeoutfor them. Since the main config setstest.setTimeout(60_000)in the test file itself this is fine, but worth noting if someone runs the standalone config and hits the default 30s timeout on a slower machine.
LGTM — the core fix is correct, well-tested (including the package-source edge case), and the CI integration is clean.
|
Review submitted with approval. The
|
2 participants
Safari RSC Client Reference Crash Report
Scope
The Safari/WebKit crash:
Root Cause Hypothesis
The minified crash maps to React RSC client-reference export lookup, roughly:
The crash means
moduleExportsisundefined.The likely failure chain is:
vinext groups many RSC client references under the virtual RSC server entry facade.
This can produce a large browser chunk like:
Safari/WebKit evaluates the resulting dynamic import graph in an order where a client reference can be observed before its module namespace is fully initialized.
React RSC later tries to read an export from
undefinedand crashes.Chrome appears to tolerate this module graph timing better, which is why the same output can work there.
Next.js Behavior
Next.js does not organize App Router client references as one large virtual RSC entry facade.
Next.js uses this pipeline:
next-flight-loadertransforms"use client"modules intoregisterClientReference(...)references.FlightClientEntryPluginwalks each App Router entry, collects reachable client components, and injects synthetic client entries.next-flight-client-entry-loadereagerly imports those client modules.ClientReferenceManifestPluginrecords each client module against real webpack entrypoint chunks.This makes client-reference chunks route/app-entry driven, not virtual-RSC-facade driven.
vinext Difference
vinext currently relies on
@vitejs/plugin-rscdefault client-reference grouping bymeta.serverChunk.For App Router builds, many references can share:
This is the artifact shape that likely exposes the Safari crash.
Bug-Relevant Invariant
vinext should avoid grouping many unrelated RSC client references into one browser chunk keyed by the virtual RSC server entry facade.
A safer shape should make browser client-reference chunks depend on client module identity or route/app-entry identity.
The closer Next.js parity target is route/app-entry based synthetic client entries plus per-route client reference manifests.