Derive RSC client refs from the RSC graph#46
Conversation
WalkthroughThis PR introduces ChangesRSC Client Reference Discovery and Manifest Filtering
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR introduces
Confidence Score: 4/5Safe to merge; the core logic is well-tested and the changes are self-contained. The src/RSCReferenceDiscoveryPlugin.ts (bundler fallback and caching trade-offs) and src/react-server-dom-webpack/cjs/react-server-dom-webpack-plugin.js (silent skip on missing blocks API) Important Files Changed
Sequence DiagramsequenceDiagram
participant WL as RSCWebpackLoader
participant RRDP as RSCReferenceDiscoveryPlugin
participant Comp as Webpack Compilation
participant RFWP as ReactFlightWebpackPlugin
Note over RRDP,Comp: apply() thisCompilation hook
RRDP->>Comp: getClientReferenceSet creates empty Set via Symbol key
Note over WL,Comp: Per-file loader execution
WL->>RRDP: recordDiscoveredClientReferenceIfNeeded
RRDP->>Comp: existingClientReferenceSet
Comp-->>RRDP: Set or undefined early return
RRDP->>WL: cacheable false
RRDP->>RRDP: hasUseClientDirective
alt has use client directive
RRDP->>Comp: refs.add resourcePath
end
Note over RRDP,Comp: processAssets STAGE_REPORT
RRDP->>Comp: emit rsc-client-references.json
Note over RFWP,Comp: processAssets STAGE_REPORT client build
RFWP->>Comp: forEach chunkGroup
Comp-->>RFWP: getBlocks or blocksIterable
RFWP->>RFWP: filter for ClientReferenceDependency
alt chunkResolvedClientFiles empty
RFWP-->>Comp: return skip chunk group
else has client-reference deps
RFWP->>Comp: recordModule to filePathToModuleMetadata
end
RFWP->>Comp: emit react-client-manifest.json
Reviews (1): Last reviewed commit: "Limit Webpack client manifest chunks to ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ): boolean { | ||
| const compilation = loaderContext._compilation as AnyCompilation | undefined; | ||
| if (!compilation) return false; | ||
|
|
||
| const refs = existingClientReferenceSet(compilation); | ||
| if (!refs) return false; | ||
|
|
||
| // Recording is a loader side effect. If this loader output is reused from a | ||
| // cache in a later watch rebuild, the side effect can be skipped and the | ||
| // emitted references can become stale. | ||
| loaderContext.cacheable?.(false); |
There was a problem hiding this comment.
cacheable(false) applied to all RSC-loader files, not just client modules
When the discovery plugin is active, every file processed by the RSC loader is marked non-cacheable — even files without 'use client'. This is intentional (the third test in rsc-reference-discovery-plugin.test.ts explicitly asserts it) to correctly detect when a file gains the directive between watch builds: a file that previously lacked 'use client' would be cached and the loader wouldn't re-run, missing the new directive.
The trade-off is that in large projects with many files passing through the RSC loader, watch-mode builds lose all incremental caching for this loader stage. It is worth documenting this expectation — either in JSDoc or in a README — so consumers know the plugin is incompatible with per-file persistent caching for the RSC loader.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| function resolveBundler(compiler: AnyCompiler) { | ||
| if (compiler.webpack) return compiler.webpack; | ||
| if (compiler.rspack) return compiler.rspack; | ||
| // eslint-disable-next-line @typescript-eslint/no-var-requires, global-require | ||
| return require('webpack'); | ||
| } |
There was a problem hiding this comment.
require('webpack') fallback can throw in Rspack-only projects
resolveBundler falls back to require('webpack') when neither compiler.webpack nor compiler.rspack is present. In a project where only Rspack is a dependency, this throws MODULE_NOT_FOUND at plugin instantiation time. A descriptive thrown error or an explicit guard would be safer than silently falling through to a potentially absent module.
| const blocks = | ||
| "function" === typeof chunkGroup.getBlocks | ||
| ? chunkGroup.getBlocks() | ||
| : chunkGroup.blocksIterable; | ||
| if (!blocks) return; | ||
| chunkResolvedClientFiles = new Set(); | ||
| var _iterator = _createForOfIteratorHelper(blocks), | ||
| _step; | ||
| try { | ||
| for (_iterator.s(); !(_step = _iterator.n()).done; ) { | ||
| const block = _step.value, | ||
| dependencies = block && block.dependencies; | ||
| if (!dependencies) continue; | ||
| for (let i = 0; i < dependencies.length; i++) { | ||
| const dep = dependencies[i]; | ||
| if ( | ||
| (dep instanceof ClientReferenceDependency || | ||
| "client-reference" === dep.type) && | ||
| resolvedClientFiles.has(dep.request) | ||
| ) | ||
| chunkResolvedClientFiles.add(dep.request); | ||
| } | ||
| } | ||
| } catch (err) { | ||
| _iterator.e(err); | ||
| } finally { | ||
| _iterator.f(); | ||
| } | ||
| if (0 === chunkResolvedClientFiles.size) return; |
There was a problem hiding this comment.
return when blocks API is unavailable can produce an incomplete client manifest
If a chunk group exposes neither getBlocks() nor blocksIterable, the code silently skips it. A client chunk group containing legitimate ClientReferenceDependency entries could be dropped from the manifest with no warning, producing hard-to-diagnose missing entries at runtime. Emitting a compilation.warnings entry — similar to the existing "Client runtime was not found" warning — when a chunk group is skipped would make this detectable.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/RSCReferenceDiscoveryPlugin.ts (2)
72-82: Caching impact:cacheable(false)runs for every module the loader touches.When the plugin is active, recording marks all RSCWebpackLoader-processed modules non-cacheable (before the directive check), which disables persistent/incremental caching for that subset on every rebuild. The inline comment explains the staleness rationale, but in watch mode this can be a measurable build-time regression. Consider monitoring incremental build times and documenting this tradeoff for downstream consumers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/RSCReferenceDiscoveryPlugin.ts` around lines 72 - 82, The loader currently calls loaderContext.cacheable?.(false) unconditionally which disables caching for every module; change it so cacheable(false) is only called when you actually will record a reference: first check resourcePath and hasUseClientDirective(source), and only if both are true call loaderContext.cacheable?.(false) before refs.add(resourcePath) (or guard the call with whatever recording flag the plugin uses); update the logic around hasUseClientDirective, resourcePath, refs.add, and loaderContext.cacheable so only modules that will be recorded are marked non-cacheable to avoid widespread cache invalidation.
66-66: TreatloaderContext._compilationas an internal (not stable) loader API; guard usage
Webpack 5 and rspack both exposethis._compilationto loaders, but it’s explicitly documented/typed as an internal underscore property (and is optional in webpack’s loader typings), so relying on it as the discovery channel should include feature detection / a clear fallback when it’s missing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/RSCReferenceDiscoveryPlugin.ts` at line 66, Guard access to the internal loader API by checking loaderContext._compilation before using it: update the code around the const compilation = loaderContext._compilation as AnyCompilation | undefined; to perform a runtime feature-detection (if (!loaderContext || !loaderContext._compilation) { /* fallback */ }) and handle the missing case with a clear fallback or graceful no-op (for example use loaderContext._compiler if available, emit a warning/log via loaderContext.emitWarning or skip discovery) so the plugin doesn't assume the underscored internal property is present; ensure all uses of the compilation variable (compilation) are conditioned on it being defined.src/react-server-dom-webpack/cjs/react-server-dom-webpack-plugin.js (1)
287-290: 💤 Low valueConsider logging when blocks are unavailable.
The early return on line 290 silently skips chunk groups when
blocksis falsy. For client builds, this could hide configuration issues or unexpected webpack structure changes. Consider adding a debug warning when this occurs to aid troubleshooting.📋 Suggested diagnostic improvement
if (!blocks) return; + // Note: Optionally log at debug level: "Skipping chunk group - no blocks found"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/react-server-dom-webpack/cjs/react-server-dom-webpack-plugin.js` around lines 287 - 290, The code currently early-returns when blocks is falsy (the branch evaluating chunkGroup.getBlocks() vs chunkGroup.blocksIterable), which silently skips chunk groups; update the early-return to emit a debug/warn log before returning — include identifying context such as chunkGroup.id or chunkGroup.name and whether this is a client build so maintainers can diagnose webpack shape/config issues; place the log immediately before the existing return in the chunkGroup processing block (where chunkGroup.getBlocks and chunkGroup.blocksIterable are used).tests/react-flight-webpack-plugin-css-order.test.ts (1)
28-30: 💤 Low valueType assertion bypasses callback signature.
Similar to the new test file, line 29 uses
as neverto force an object with an extratype: 'client-reference'field through the callback signature that expectsArray<{ request: string; userRequest: string }>. While functional at runtime with the mocked implementation, this obscures the type mismatch.🔧 More explicit type handling
callback(null, [ - { request: clientFile, type: 'client-reference', userRequest: './ClientComponent.js' } as never, + { request: clientFile, userRequest: './ClientComponent.js' } as { request: string; userRequest: string }, ]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/react-flight-webpack-plugin-css-order.test.ts` around lines 28 - 30, The test currently circumvents the callback signature by using "as never" when returning an object with an extra type: 'client-reference' field; update the mock to respect types instead of bypassing them by either widening the callback's expected item type to include the optional "type" property or by constructing an object that matches Array<{ request: string; userRequest: string }>, e.g. remove the "as never" and adjust the returned shape or the callback generic so the item containing clientFile and userRequest: './ClientComponent.js' is correctly typed without a forced cast.tests/react-flight-webpack-plugin-client-reference-chunks.test.ts (1)
28-44: 💤 Low valueType assertion bypasses callback signature.
The callback type expects
Array<{ request: string; userRequest: string }>, but line 42 passes an object with an additionaltype: 'client-reference'field. Theas nevercast forces this through TypeScript's type checking. While this works at runtime (sinceresolveAllClientFilesis mocked), it obscures the type mismatch and could be confusing for future maintainers.🔧 More explicit type handling
Consider either:
- Updating the mock signature to explicitly allow the extra field, or
- Removing the
typefield if it's not actually used by downstream code in the testcallback(null, [ - { request: clientFile, type: 'client-reference', userRequest: './ErrorBoundary.tsx' } as never, + { request: clientFile, userRequest: './ErrorBoundary.tsx' } as { request: string; userRequest: string }, ]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/react-flight-webpack-plugin-client-reference-chunks.test.ts` around lines 28 - 44, The mock for plugin.resolveAllClientFiles returns an object with an extra type field and casts it with "as never", hiding a type mismatch; update the mock to match the declared callback signature by either removing the extra type property from the returned object (return { request: clientFile, userRequest: './ErrorBoundary.tsx' }) or change the mock's callback generic/type to accept the additional field so you no longer need "as never" on the resolved array; locate resolveAllClientFiles and the callback in the test and make one of those two changes so the returned Array<{ request: string; userRequest: string }> is satisfied without type assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/clientReferences.ts`:
- Line 18: The USE_CLIENT_REGEX currently requires a newline or end-of-input
after the 'use client' directive so same-line statements are missed; update
USE_CLIENT_REGEX so it no longer mandates a newline/end but instead allows an
optional semicolon followed by other characters on the same line (e.g., use a
lookahead that accepts whitespace or end-of-input rather than \n), matching how
hasUseClientDirective in the react plugin detects directives so single-line
"'use client'; export ..." is recognized.
---
Nitpick comments:
In `@src/react-server-dom-webpack/cjs/react-server-dom-webpack-plugin.js`:
- Around line 287-290: The code currently early-returns when blocks is falsy
(the branch evaluating chunkGroup.getBlocks() vs chunkGroup.blocksIterable),
which silently skips chunk groups; update the early-return to emit a debug/warn
log before returning — include identifying context such as chunkGroup.id or
chunkGroup.name and whether this is a client build so maintainers can diagnose
webpack shape/config issues; place the log immediately before the existing
return in the chunkGroup processing block (where chunkGroup.getBlocks and
chunkGroup.blocksIterable are used).
In `@src/RSCReferenceDiscoveryPlugin.ts`:
- Around line 72-82: The loader currently calls loaderContext.cacheable?.(false)
unconditionally which disables caching for every module; change it so
cacheable(false) is only called when you actually will record a reference: first
check resourcePath and hasUseClientDirective(source), and only if both are true
call loaderContext.cacheable?.(false) before refs.add(resourcePath) (or guard
the call with whatever recording flag the plugin uses); update the logic around
hasUseClientDirective, resourcePath, refs.add, and loaderContext.cacheable so
only modules that will be recorded are marked non-cacheable to avoid widespread
cache invalidation.
- Line 66: Guard access to the internal loader API by checking
loaderContext._compilation before using it: update the code around the const
compilation = loaderContext._compilation as AnyCompilation | undefined; to
perform a runtime feature-detection (if (!loaderContext ||
!loaderContext._compilation) { /* fallback */ }) and handle the missing case
with a clear fallback or graceful no-op (for example use loaderContext._compiler
if available, emit a warning/log via loaderContext.emitWarning or skip
discovery) so the plugin doesn't assume the underscored internal property is
present; ensure all uses of the compilation variable (compilation) are
conditioned on it being defined.
In `@tests/react-flight-webpack-plugin-client-reference-chunks.test.ts`:
- Around line 28-44: The mock for plugin.resolveAllClientFiles returns an object
with an extra type field and casts it with "as never", hiding a type mismatch;
update the mock to match the declared callback signature by either removing the
extra type property from the returned object (return { request: clientFile,
userRequest: './ErrorBoundary.tsx' }) or change the mock's callback generic/type
to accept the additional field so you no longer need "as never" on the resolved
array; locate resolveAllClientFiles and the callback in the test and make one of
those two changes so the returned Array<{ request: string; userRequest: string
}> is satisfied without type assertions.
In `@tests/react-flight-webpack-plugin-css-order.test.ts`:
- Around line 28-30: The test currently circumvents the callback signature by
using "as never" when returning an object with an extra type: 'client-reference'
field; update the mock to respect types instead of bypassing them by either
widening the callback's expected item type to include the optional "type"
property or by constructing an object that matches Array<{ request: string;
userRequest: string }>, e.g. remove the "as never" and adjust the returned shape
or the callback generic so the item containing clientFile and userRequest:
'./ClientComponent.js' is correctly typed without a forced cast.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 30753a2e-164c-453c-b6e8-8a2b59bb1da9
📒 Files selected for processing (9)
package.jsonsrc/RSCReferenceDiscoveryPlugin.tssrc/WebpackLoader.tssrc/clientReferences.tssrc/react-server-dom-rspack/shared.tssrc/react-server-dom-webpack/cjs/react-server-dom-webpack-plugin.jstests/react-flight-webpack-plugin-client-reference-chunks.test.tstests/react-flight-webpack-plugin-css-order.test.tstests/rsc-reference-discovery-plugin.test.ts
| */ | ||
| export const DEFAULT_CLIENT_REFERENCES_INCLUDE = /\.[cm]?[jt]sx?$/; | ||
|
|
||
| const USE_CLIENT_REGEX = /^\s*['"]use client['"]\s*;?\s*(?:\n|$)/; |
There was a problem hiding this comment.
Directive regex misses same-line 'use client' followed by code.
USE_CLIENT_REGEX requires a newline or end-of-input after the (optional) semicolon, so a valid single-line directive like 'use client'; export const x = 1 is not detected. This diverges from the acorn-based directive-prologue detection used by the React Flight webpack plugin (hasUseClientDirective in react-server-dom-webpack-plugin.js), which accepts a trailing statement on the same line — meaning the same module can be treated as a client reference by the manifest path but missed by discovery.
🛠️ Proposed fix
-const USE_CLIENT_REGEX = /^\s*['"]use client['"]\s*;?\s*(?:\n|$)/;
+const USE_CLIENT_REGEX = /^\s*['"]use client['"]\s*(?:;|\n|$)/;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const USE_CLIENT_REGEX = /^\s*['"]use client['"]\s*;?\s*(?:\n|$)/; | |
| const USE_CLIENT_REGEX = /^\s*['"]use client['"]\s*(?:;|\n|$)/; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/clientReferences.ts` at line 18, The USE_CLIENT_REGEX currently requires
a newline or end-of-input after the 'use client' directive so same-line
statements are missed; update USE_CLIENT_REGEX so it no longer mandates a
newline/end but instead allows an optional semicolon followed by other
characters on the same line (e.g., use a lookahead that accepts whitespace or
end-of-input rather than \n), matching how hasUseClientDirective in the react
plugin detects directives so single-line "'use client'; export ..." is
recognized.
|
I could not push directly to the fork branch despite |
|
#47 landed. Fixes are there. |
2 participants
Important
This PR is paired with shakacode/react_on_rails#3556 and should be reviewed together with that PR. It should not be merged on its own.
Summary
RSCReferenceDiscoveryPluginso downstream builds can consume exact refs from the RSC graphThis supports the graph-derived client reference proposal in shakacode/react_on_rails#3553.
Why
Broad
clientReferencesscanning can treat unrelated'use client'files as RSC client references. For Rspack, those false positives become injected dynamic imports. For Webpack manifests, unrelated chunk groups can make normal browser entries eligible for RSC client-reference loading.This PR adds package-level support for using the RSC graph as the source of truth and includes a targeted Webpack client-manifest chunk filtering fix for the over-inclusion case found while validating that path.
Verification
yarn buildyarn testResult: 15 suites passed, 99 tests passed.