Derive RSC client refs from the RSC graph#47
Conversation
|
Warning Review limit reached
More reviews will be available in 42 minutes and 13 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✨ 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 |
Code Review: Derive RSC client refs from the RSC graphOverviewThis PR introduces The implementation is solid overall, with good test coverage for the new behavior. A few things worth discussing: Correctness
Chunk group scoping logic is correct. Filtering Concerns1. Performance: every loader invocation is marked non-cacheable when the plugin is active
The comment explains the reasoning (stale discovery if cache is reused), but users who opt into this plugin need to understand the tradeoff. Consider documenting this in the plugin's JSDoc/README, or exploring a finer-grained invalidation strategy (e.g. storing a content hash of the recorded set and invalidating only when it changes). 2. Private webpack API:
3. The new test file has a single case. Given that 4. The code falls back to Minor nits
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // 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.
This marks the loader output non-cacheable for every file when the plugin is active, not just files with a 'use client' directive. The intent (preventing stale discovery in watch rebuilds) is clear from the comment, but the blast radius is the entire RSC compilation — no loader result will ever be reused from webpack's persistent cache as long as this plugin is registered.
For large applications this could meaningfully hurt incremental build times. Worth surfacing in the plugin's docs/JSDoc so adopters can make an informed tradeoff. If a finer-grained signal is ever needed, one approach is to store a hash of the collected paths in the compilation and only invalidate when it changes.
| loaderContext: LoaderContextWithCompilation, | ||
| source: string | Buffer, | ||
| ): boolean { | ||
| const compilation = loaderContext._compilation as AnyCompilation | undefined; |
There was a problem hiding this comment.
_compilation is a private internal webpack property — it's not part of the public loader context API and could disappear in a future webpack major version. There's no official alternative today, so using it is probably unavoidable, but a comment like // webpack does not expose a public API for this would flag it for future maintainers rather than letting it look like an intentional choice.
| it('recognizes a directive followed by code on the same line', () => { | ||
| expect(hasUseClientDirective("'use client'; export const answer = 42;\n")).toBe(true); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This file only covers one case. Since hasUseClientDirective is now the shared implementation for both webpack and rspack paths, a few more cases here would prevent regressions:
// double-quote variant
expect(hasUseClientDirective('"use client";\nexport default function C() {}')).toBe(true);
// Buffer input
expect(hasUseClientDirective(Buffer.from("'use client';\n"))).toBe(true);
// directive preceded by a block comment
expect(hasUseClientDirective('/* @generated */\n\'use client\';\n')).toBe(true);
// directive at EOF, no trailing newline
expect(hasUseClientDirective("'use client'")).toBe(true);
// negative: plain server module
expect(hasUseClientDirective("export default function Server() {}")).toBe(false);
| let chunkResolvedClientFiles = resolvedClientFiles; | ||
| if (!_this.isServer) { | ||
| const blocks = | ||
| "function" === typeof chunkGroup.getBlocks |
There was a problem hiding this comment.
The blocksIterable branch is a fallback for older webpack 5 releases where getBlocks() did not yet exist as a method. A short comment clarifying the version split (e.g. // getBlocks() was introduced in webpack 5.x; blocksIterable is the earlier equivalent) would prevent a future reader from wondering whether the fallback is dead code or still load-bearing.
| hooks: { | ||
| processAssets: { | ||
| tap: (_options: unknown, callback: () => void) => { | ||
| processAssetCallbacks.push(callback); |
There was a problem hiding this comment.
compilation.warnings is inferred as never[] from the [] literal, so TypeScript won't catch type mismatches on the warning elements. Typing it as unknown[] (or Array<{ message: string }>) would give the String(warnings[0]) assertion below a bit more type safety.
Code Review: Derive RSC client refs from the RSC graphOverall this is a well-structured PR with sound design and good test coverage. A few items worth addressing before merge. CorrectnessSame-line directive detection ✅ Windows line endings (
Performance
Warning verbosityIn Test gaps
Minor
|
| */ | ||
| export const DEFAULT_CLIENT_REFERENCES_INCLUDE = /\.[cm]?[jt]sx?$/; | ||
|
|
||
| const USE_CLIENT_REGEX = /^\s*['"]use client['"]\s*(?:;|\n|$)/; |
There was a problem hiding this comment.
The \r\n (Windows) case works but only implicitly: \s* absorbs the \r, then \n matches. A reader might assume \r\n files fail. Consider making the intent explicit:
| const USE_CLIENT_REGEX = /^\s*['"]use client['"]\s*(?:;|\n|$)/; | |
| const USE_CLIENT_REGEX = /^\s*['"]use client['"]\s*(?:;|\r?\n|$)/; |
This also covers bare \r (old Mac line endings) which \s* + \n would silently miss if the file has no trailing \n.
| // watch rebuild, discovery can miss files that gained a `"use client"` | ||
| // directive. This intentionally disables caching for every RSC loader input | ||
| // while the plugin is active. | ||
| loaderContext.cacheable?.(false); |
There was a problem hiding this comment.
cacheable(false) is called here, before the hasUseClientDirective check on the next line, so every file passing through the RSC loader is marked non-cacheable while the plugin is active—not just client modules. The comment above explains why this is intentional (watch-rebuild correctness), but it's worth surfacing this in the plugin's public-facing JSDoc too, since users won't read the loader source to understand why incremental builds are slower after adding this plugin.
| ): boolean { | ||
| // Webpack/Rspack do not expose a public loader API for the active compilation. | ||
| // `_compilation` is the only available bridge for recording loader side effects. | ||
| const compilation = loaderContext._compilation as AnyCompilation | undefined; |
There was a problem hiding this comment.
_compilation is a private/internal webpack field. It has existed across all webpack 5 releases so far, but it is not part of the public API contract. If this field is ever renamed (it was previously _module in webpack 4), recordDiscoveredClientReferenceIfNeeded silently becomes a no-op (returns false) rather than throwing—which could cause missed client references without any diagnostic. A minimum webpack version comment here (e.g. // works on webpack >= 5.0) or a one-time warning when _compilation is undefined but resourcePath is defined would make breakage visible.
| if (!blocks) { | ||
| const chunkGroupName = | ||
| chunkGroup.name || chunkGroup.id || "(unnamed)"; | ||
| compilation.warnings.push( |
There was a problem hiding this comment.
This warning is emitted once per chunk group that lacks getBlocks/blocksIterable. On older webpack 5 builds with many entry points, this could produce dozens of identical warnings per build. Consider deduplicating with a per-compilation flag:
if (!compilation._rscBlocksWarningEmitted) {
compilation._rscBlocksWarningEmitted = true;
compilation.warnings.push(new webpack.WebpackError(...));
}Or at minimum, mention the webpack version that introduced getBlocks so users know whether to expect this warning.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0d78c86e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "./RSCReferenceDiscoveryPlugin": { | ||
| "types": "./dist/RSCReferenceDiscoveryPlugin.d.ts", | ||
| "default": "./dist/RSCReferenceDiscoveryPlugin.js" |
There was a problem hiding this comment.
This export points at a new dist artifact, but scripts.build-if-needed still only tests the old dist files in package.json:82. In a checkout that already has dist from the previous version, yarn prepack/prepare will skip yarn build, so the packed or git-installed package can contain this export pointing to a missing dist/RSCReferenceDiscoveryPlugin.js; add the new JS and declaration files to the freshness check.
Useful? React with 👍 / 👎.
Code Review — PR #47: Derive RSC client refs from the RSC graphOverviewThis PR is a clean follow-up to #46. It introduces FindingsMedium — Directive regex misses trailing inline comment
Medium —
|
| */ | ||
| export const DEFAULT_CLIENT_REFERENCES_INCLUDE = /\.[cm]?[jt]sx?$/; | ||
|
|
||
| const USE_CLIENT_REGEX = /^\s*['"]use client['"]\s*(?:;|\n|$)/; |
There was a problem hiding this comment.
The suffix alternation (?:;|\n|$) does not match a trailing inline comment, so 'use client' // eslint-disable-line would be silently missed. That pattern is unusual but plausible (code generators, linter annotations). Two options:
Option A — drop the suffix constraint (safe after stripProlog puts you at the first real token):
| const USE_CLIENT_REGEX = /^\s*['"]use client['"]\s*(?:;|\n|$)/; | |
| const USE_CLIENT_REGEX = /^\s*['"]use client['"]/; |
Option B — allow an optional line comment before the newline/EOF:
| const USE_CLIENT_REGEX = /^\s*['"]use client['"]\s*(?:;|\n|$)/; | |
| const USE_CLIENT_REGEX = /^\s*['"]use client['"]\s*(?:;|(?:\/\/[^\n]*)?\s*(?:\n|$))/; |
| // watch rebuild, discovery can miss files that gained a `"use client"` | ||
| // directive. This intentionally disables caching for every RSC loader input | ||
| // while the plugin is active. | ||
| loaderContext.cacheable?.(false); |
There was a problem hiding this comment.
cacheable(false) is called before hasUseClientDirective, so every file processed by the RSC loader loses persistent caching when the plugin is active — not just "use client" modules. The comment explains the correctness rationale, but the blast radius on large codebases (all RSC-loader inputs re-processed on every watch rebuild) is worth calling out in the public docs so users can make an informed decision about enabling this plugin.
| // Webpack/Rspack do not expose a public loader API for the active compilation. | ||
| // `_compilation` is the only available bridge for recording loader side effects. | ||
| const compilation = loaderContext._compilation as AnyCompilation | undefined; | ||
| if (!compilation) return false; |
There was a problem hiding this comment.
When webpack runs loaders inside worker threads (e.g. via thread-loader), the loader context is serialised into the worker and _compilation is almost certainly undefined. The function returns false silently, so discovery stops working without any diagnostic. If _compilation is absent while the RSC loader is otherwise running normally, emitting a one-time compilation.warnings entry (or a console.warn as a fallback) would make this failure mode visible to the user.
// _compilation may be undefined in worker-thread contexts (e.g. thread-loader).
const compilation = loaderContext._compilation as AnyCompilation | undefined;
if (!compilation) {
// No warning here yet — see note above about thread-loader compatibility.
return false;
}
| relativeRefs: refs.map((file) => | ||
| path.relative(compiler.context, file).replace(/\\/g, '/'), |
There was a problem hiding this comment.
When a client file lives outside compiler.context (e.g. a symlinked package in a monorepo), path.relative produces a ../-prefixed path. The emitted JSON will mix within-context relative paths with escape paths, which may surprise consumers that assume all relativeRefs entries are rooted inside the project. Consider documenting this behaviour, or adding a warning/guard:
relativeRefs: refs.map((file) => {
const rel = path.relative(compiler.context, file).replace(/\\/g, '/');
// rel starts with '../' when the file is outside compiler.context
return rel;
}),
Greptile SummaryThis PR introduces graph-driven RSC client reference discovery: a new
Confidence Score: 4/5Safe to merge; the block-based chunk filtering and plugin discovery mechanism are well-tested and the directive regex fix is correct. The core changes — graph-derived client reference discovery, the corrected hasUseClientDirective regex, and the chunk-group scoping in the webpack plugin — are all well-exercised by the new test suite. The only finding is a minor diagnostic gap in the warning message when chunkGroup.id === 0, which would not affect build output or runtime behavior. The chunkGroup.name || chunkGroup.id || "(unnamed)" line in react-server-dom-webpack-plugin.js is worth a second look for the id === 0 edge case. Important Files Changed
Sequence DiagramsequenceDiagram
participant WL as WebpackLoader
participant RRDP as RSCReferenceDiscoveryPlugin
participant Comp as Compilation
participant WP as ReactFlightWebpackPlugin
Note over RRDP,Comp: apply(compiler) — thisCompilation hook
RRDP->>Comp: getClientReferenceSet(compilation) initialises Symbol-keyed Set
Note over WL,Comp: Each module processed by loader
WL->>RRDP: recordDiscoveredClientReferenceIfNeeded(this, source)
RRDP->>Comp: existingClientReferenceSet(compilation)
alt Plugin active (Set exists)
RRDP->>WL: cacheable(false)
alt hasUseClientDirective(source)
RRDP->>Comp: refs.add(resourcePath)
end
end
Note over RRDP,Comp: processAssets STAGE_REPORT
RRDP->>Comp: getClientReferenceSet(compilation)
RRDP->>Comp: emitAsset rsc-client-references.json
Note over WP,Comp: processAssets chunkGroups loop client build
loop each chunkGroup
WP->>WP: "blocks = chunkGroup.getBlocks() or blocksIterable"
alt blocks unavailable
WP->>Comp: warnings.push(WebpackError)
else blocks available
WP->>WP: build chunkResolvedClientFiles from ClientReferenceDependency in blocks
alt chunkResolvedClientFiles not empty
WP->>Comp: recordModule to filePathToModuleMetadata
end
end
end
WP->>Comp: emitAsset react-client-manifest.json
Reviews (1): Last reviewed commit: "Address RSC review followups" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!blocks) { | ||
| if (!missingClientReferenceBlocksWarningEmitted) { | ||
| missingClientReferenceBlocksWarningEmitted = true; |
There was a problem hiding this comment.
(unnamed) when id is 0
chunkGroup.id can be 0 in webpack (IDs are integers starting from zero), but 0 is falsy, so chunkGroup.name || chunkGroup.id || "(unnamed)" skips id === 0 and falls through to "(unnamed)". A chunk group with no name and ID 0 would always appear as "(unnamed)" in the warning, making the message misleading to debuggers.
Code Review: Derive RSC client refs from the RSC graphOverall: Well-structured PR with thorough test coverage. The directive-detection fix is a real correctness improvement and the chunk-group filtering change addresses a genuine manifest-pollution issue. A few things worth discussing: What's good
Issues / questions1.
|
| // watch rebuild, discovery can miss files that gained a `"use client"` | ||
| // directive. This intentionally disables caching for every RSC loader input | ||
| // while the plugin is active. | ||
| loaderContext.cacheable?.(false); |
There was a problem hiding this comment.
The cacheable(false) call happens before the hasUseClientDirective check, so every file that passes through the RSC loader is marked non-cacheable while the plugin is active — not just 'use client' files. This is intentional and matches the Rspack loader's approach, but the performance cost scales linearly with the number of RSC-processed modules. In a large app this could noticeably slow watch-mode rebuilds. Consider surfacing this trade-off in the plugin's public JSDoc comment so consumers know what they're opting into.
|
|
||
| const RSC_CLIENT_REFERENCES_KEY: symbol = Symbol.for( | ||
| 'react-on-rails-rsc.discoveredClientReferences', | ||
| ); |
There was a problem hiding this comment.
Symbol.for looks up a key in the global symbol registry, so if two RSCReferenceDiscoveryPlugin instances are applied to the same compilation (e.g. a multi-compiler build with shared state, or a duplicated dependency in a monorepo), both will read/write the same Set. The second plugin's processAssets handler will include references accumulated by both passes and emit them to its own filename — likely harmless in practice, but the shared-state behaviour is surprising. Adding a short note in the JSDoc would help future maintainers understand the intent.
| if (!missingClientReferenceBlocksWarningEmitted) { | ||
| missingClientReferenceBlocksWarningEmitted = true; | ||
| const chunkGroupName = | ||
| chunkGroup.name || chunkGroup.id || "(unnamed)"; | ||
| compilation.warnings.push( | ||
| new webpack.WebpackError( | ||
| "Client reference blocks were unavailable for chunk group " + | ||
| chunkGroupName + | ||
| ". React Server Components client manifest entries for this chunk group were skipped." | ||
| ) | ||
| ); |
There was a problem hiding this comment.
The "emit once" behaviour is intentional and tested, but the warning message names chunkGroupName — the first failing chunk group — which makes the problem sound isolated when it may affect many groups (e.g. on a webpack build that exposes neither getBlocks nor blocksIterable). A user seeing this warning has no way to know whether one group or all of them are affected.
Consider a generic message that avoids naming a specific group:
| if (!missingClientReferenceBlocksWarningEmitted) { | |
| missingClientReferenceBlocksWarningEmitted = true; | |
| const chunkGroupName = | |
| chunkGroup.name || chunkGroup.id || "(unnamed)"; | |
| compilation.warnings.push( | |
| new webpack.WebpackError( | |
| "Client reference blocks were unavailable for chunk group " + | |
| chunkGroupName + | |
| ". React Server Components client manifest entries for this chunk group were skipped." | |
| ) | |
| ); | |
| compilation.warnings.push( | |
| new webpack.WebpackError( | |
| "Client reference blocks were unavailable for one or more chunk groups" + | |
| " (first affected: " + chunkGroupName + ")." + | |
| " React Server Components client manifest entries for affected chunk groups were skipped." | |
| ) | |
| ); |
| s = nl === -1 ? '' : s.slice(nl + 1); | ||
| } | ||
| const stripped = s.replace(LEADING_COMMENTS, ''); | ||
| if (stripped !== s) s = stripped; |
There was a problem hiding this comment.
The if (stripped !== s) guard is redundant — s = stripped is a no-op when the two values are already identical, and the comparison itself is O(n) on the string length. Just s = stripped is simpler.
| if (stripped !== s) s = stripped; | |
| s = s.replace(LEADING_COMMENTS, ''); |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab9cb9e7fa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| */ | ||
| export const DEFAULT_CLIENT_REFERENCES_INCLUDE = /\.[cm]?[jt]sx?$/; | ||
|
|
||
| const USE_CLIENT_REGEX = /^\s*['"]use client['"]\s*(?:;|\r?\n|$)/; |
There was a problem hiding this comment.
When a client module starts with another directive such as 'use strict';\n'use client';, this regex returns false because it only accepts use client as the first directive. The RSC webpack loader in react-server-dom-webpack-node-loader.production.js scans every directive statement before the first non-directive, so such a file is still transformed as a client module, but RSCReferenceDiscoveryPlugin will omit it from rsc-client-references.json; keep scanning directive statements until the prologue ends instead of matching only the first one.
Useful? React with 👍 / 👎.
PR #47 Review: Derive RSC client refs from the RSC graphOverview This PR introduces Strengths
Issues Moderate Chunk-group filtering silently drops modules with non-standard dependency types. When Minor
|
| */ | ||
| export const DEFAULT_CLIENT_REFERENCES_INCLUDE = /\.[cm]?[jt]sx?$/; | ||
|
|
||
| const USE_CLIENT_REGEX = /^\s*['"]use client['"]\s*(?:;|\r?\n|$)/; |
There was a problem hiding this comment.
The alternation \r?\n covers \r\n (CRLF) and \n (LF) but not a bare \r (classic Mac, pre-OS X). Vanishingly rare in the wild, but easy to cover:
| const USE_CLIENT_REGEX = /^\s*['"]use client['"]\s*(?:;|\r?\n|$)/; | |
| const USE_CLIENT_REGEX = /^\s*['"]use client['"]\s*(?:;|\r\n?|$)/; |
\r\n? matches \r\n (CRLF) or \r alone (old Mac), and the existing $ already covers LF-less EOF.
| const stripped = s.replace(LEADING_COMMENTS, ''); | ||
| if (stripped !== s) s = stripped; |
There was a problem hiding this comment.
The if guard is a no-op — assigning s = stripped when they're equal changes nothing. Simplify to a single assignment:
| const stripped = s.replace(LEADING_COMMENTS, ''); | |
| if (stripped !== s) s = stripped; | |
| s = s.replace(LEADING_COMMENTS, ''); |
| const payload = { | ||
| version: 1, | ||
| compilerContext: compiler.context, | ||
| count: refs.length, |
There was a problem hiding this comment.
count duplicates refs.length. If a consumer parses this JSON and count ever disagrees with refs.length (e.g. after a transform), it becomes a source of confusion. Consider dropping it and letting consumers derive the value:
| count: refs.length, |
(i.e. remove the count line — callers can use payload.refs.length)
Code Review: Derive RSC client refs from the RSC graphOverall: This is a well-structured PR that upgrades client-reference discovery from regex to proper AST parsing, adds a public plugin for emitting the reference list, and fixes chunk-group scoping in the React Flight webpack plugin. Test coverage is good and the intent of each change is clear. Correctness / Potential Bugs1. No error boundary around
try {
const program = acorn.parse(text, { ... }) as ParsedProgram;
// ... directive loop ...
} catch {
return false;
}2. The new chunk-group scoping code in Performance3. Full-file parse for every file containing The early 4. This is documented and intentional, but the consequence is significant: in watch mode, all RSC loader inputs lose incremental-build cache reuse for the lifetime of the plugin. Worth benchmarking once the feature lands in a real project and revisiting if overhead is unacceptable (e.g., targeted invalidation instead of blanket non-cacheability). Maintainability5. Adding two more Minor / Style
Positive highlights
|
| const text = Buffer.isBuffer(source) ? source.toString('utf8') : source; | ||
| if (!text.includes('use client')) return false; | ||
|
|
||
| const program = acorn.parse(text, { |
There was a problem hiding this comment.
Bug risk: no error boundary around acorn.parse()
acorn-loose is designed to tolerate most syntax errors, but it can still throw on certain inputs (raw null bytes, extremely long strings, files starting with an unexpected BOM variant, etc.). When it throws here the exception propagates through the webpack loader pipeline and aborts the entire build.
Wrapping in a try/catch and returning false is the safe default — if we cannot parse the file we don't know whether it has a directive, so we skip it:
| const program = acorn.parse(text, { | |
| let program: ParsedProgram; | |
| try { | |
| program = acorn.parse(text, { | |
| allowHashBang: true, | |
| ecmaVersion: 'latest', | |
| sourceType: 'module', | |
| }) as ParsedProgram; | |
| } catch { | |
| return false; | |
| } |
| /** Check whether `source` starts with a `"use client"` directive. */ | ||
| export function hasUseClientDirective(source: string | Buffer): boolean { | ||
| const text = Buffer.isBuffer(source) ? source.toString('utf8') : source; | ||
| if (!text.includes('use client')) return false; |
There was a problem hiding this comment.
Performance: full-file parse on every "use client" hit
The text.includes('use client') guard correctly skips pure server modules, but any file that mentions 'use client' in a comment, an error message, or a JSDoc will be handed to acorn for a complete AST parse. In a large monorepo this can add measurable time to cold builds.
Two lighter alternatives:
- Scan only the first N bytes (directives must appear before any non-string-literal statement, so a 2 KB window covers every real-world case).
- Pre-check for a quote+
use client+quote pattern with a fast regex before invoking acorn.
If the current full-parse approach is intentional, please add a note to the JSDoc so future maintainers know the cost is accepted.
| // watch rebuild, discovery can miss files that gained a `"use client"` | ||
| // directive. This intentionally disables caching for every RSC loader input | ||
| // while the plugin is active. | ||
| loaderContext.cacheable?.(false); |
There was a problem hiding this comment.
Watch-mode performance: blanket cacheable(false) covers all RSC loader inputs
The comment above explains why this is necessary for correctness, which is good. But the practical consequence in a large project is that every file processed by the RSC loader loses incremental-build caching for the entire compilation — not just the files that actually have 'use client'.
Worth benchmarking on a real project and tracking as a follow-up. A targeted approach (e.g., invalidating only the cached entries for files that gain or lose a 'use client' directive across rebuilds) could recover most of the cache benefit while preserving correctness.
| return refs; | ||
| } | ||
|
|
||
| function existingClientReferenceSet(compilation: AnyCompilation): Set<string> | undefined { |
There was a problem hiding this comment.
Duplication between existingClientReferenceSet and getClientReferenceSet
Both helpers do the same compilation[RSC_CLIENT_REFERENCES_KEY] instanceof Set check. The only difference is whether they create the set when it's missing. A single helper avoids the duplication:
function clientReferenceSet(compilation: AnyCompilation, create: true): Set<string>;
function clientReferenceSet(compilation: AnyCompilation, create: false): Set<string> | undefined;
function clientReferenceSet(compilation: AnyCompilation, create: boolean): Set<string> | undefined {
const existing = compilation[RSC_CLIENT_REFERENCES_KEY];
if (existing instanceof Set) return existing as Set<string>;
if (!create) return undefined;
const refs = new Set<string>();
compilation[RSC_CLIENT_REFERENCES_KEY] = refs;
return refs;
}Not a blocker, just reduces the surface area to keep in sync.
| var _iterator = _createForOfIteratorHelper(blocks), | ||
| _step; | ||
| try { | ||
| for (_iterator.s(); !(_step = _iterator.n()).done; ) { |
There was a problem hiding this comment.
blocksIterable fallback path is untested
The compatibility shim for older webpack 5 builds (blocksIterable) is not exercised by any of the new tests. A chunk-group mock that omits getBlocks and only exposes blocksIterable would confirm the fallback works:
{
blocksIterable: clientReferenceBlocks, // no getBlocks
chunks: [clientChunk],
}Without this, the fallback could be silently broken (e.g., if blocksIterable needs to be iterated differently from getBlocks()).
| const dep = dependencies[i]; | ||
| if ( | ||
| (dep instanceof ClientReferenceDependency || | ||
| "client-reference" === dep.type) && |
There was a problem hiding this comment.
Warning message could name the affected chunk group
When chunkGroup.name is set, including it in the warning message makes it much easier for users to identify which entry point is missing block metadata:
compilation.warnings.push(
new webpack.WebpackError(
"Client reference blocks were unavailable for chunk group" +
(chunkGroup.name ? ` "${chunkGroup.name}"` : "") +
". React Server Components client manifest entries for this chunk group were skipped."
)
);This also enables removing the missingClientReferenceBlocksWarningEmitted dedup flag if the message is now per-group (though the flag is still useful to avoid spam when many anonymous groups are affected).
| const charCode = source.charCodeAt(i); | ||
| if (charCode === 9 || charCode === 11 || charCode === 12 || charCode === 32) { | ||
| i += 1; |
There was a problem hiding this comment.
The charCodeAt comparisons are hard to read at a glance. Named constants (or at least inline comments) would make the intent immediately clear:
| const charCode = source.charCodeAt(i); | |
| if (charCode === 9 || charCode === 11 || charCode === 12 || charCode === 32) { | |
| i += 1; | |
| if (charCode === 9 /* \t */ || charCode === 11 /* \v */ || charCode === 12 /* \f */ || charCode === 32 /* space */) { |
Alternatively, a simple Unicode escape range check (charCode < 33 && charCode !== 10 && charCode !== 13) would be more concise, though the explicit list is more precise about the allowed whitespace class.
| } | ||
|
|
||
| /** Check whether `source` starts with a `"use client"` directive. */ | ||
| export function hasUseClientDirective(source: string | Buffer): boolean { |
There was a problem hiding this comment.
The old shared.ts version explicitly stripped the BOM before testing (if (s.charCodeAt(0) === 0xfeff) s = s.slice(1)). The new Acorn-based path removes that explicit step.
Acorn handles the BOM (treats it as whitespace), and since both text and the AST end positions are relative to the same BOM-inclusive string, hasDirectiveTerminator should work correctly. But there's no test to confirm this. A test would guard against a regression if acorn-loose ever changes BOM position accounting:
it('recognizes directives in BOM-prefixed files', () => {
// U+FEFF byte-order mark — previously stripped explicitly, now handled by acorn
expect(hasUseClientDirective('\'use client\';\n')).toBe(true);
});
| jest.spyOn(acorn, 'parse').mockImplementationOnce(() => { | ||
| throw new Error('parse failed'); | ||
| }); | ||
|
|
There was a problem hiding this comment.
The jest.spyOn(acorn, 'parse') spy works in Jest's CommonJS mode because both the test file and src/clientReferences.ts receive the same module instance, making the parse property writable. In ESM (or if acorn-loose ever ships a native ES module build), the namespace object is sealed and spyOn would throw a runtime error — or worse, silently pass a test that exercises the wrong code path.
A more resilient approach is to provoke a real parse error without mocking:
it('returns false when directive parsing fails', () => {
// An unclosed template literal causes acorn-loose to throw
expect(hasUseClientDirective("'use client';\n`unclosed")).toBe(false);
});If the spy is kept, add a comment documenting the CommonJS-instance assumption so a future ESM migration can catch the fragility early.
| 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); | ||
| } |
There was a problem hiding this comment.
The dep instanceof ClientReferenceDependency check only matches deps created in this file's class. The "client-reference" === dep.type fallback is meant to cover Rspack or third-party deps with the same semantic type. However, dep.request on a third-party dependency might be a relative module specifier rather than the resolved absolute path stored in resolvedClientFiles, causing the resolvedClientFiles.has(dep.request) lookup to silently miss it.
If this path is intentionally future-proofing for Rspack, it's worth a brief comment; if it's unreachable in the current codebase it can be removed to avoid ambiguity.
|
|
||
| apply(compiler: AnyCompiler): void { | ||
| const pluginName = 'RSCReferenceDiscoveryPlugin'; | ||
| const bundler = resolveBundler(compiler); | ||
|
|
||
| compiler.hooks.thisCompilation.tap(pluginName, (compilation) => { | ||
| getClientReferenceSet(compilation); | ||
|
|
||
| compilation.hooks.processAssets.tap( | ||
| { name: pluginName, stage: bundler.Compilation.PROCESS_ASSETS_STAGE_REPORT }, | ||
| () => { | ||
| const refs = Array.from(getClientReferenceSet(compilation)).sort(); |
There was a problem hiding this comment.
cacheable(false) fires for every RSC-loader input while the plugin is active, even for server-only modules that will never have "use client". This is documented in the class JSDoc, but the cost compounds in large codebases where most files are server components.
One possible middle-ground: disable caching only on the first call per resource path and re-enable it once you know the directive is absent. That's tricky to implement correctly, so the current conservative approach is fine — just worth calling out explicitly in the option docs so users know to expect a slower incremental build when the plugin is installed.
|
See inline review comments for specifics. |
Code Review Summary - This PR centralizes hasUseClientDirective under Acorn (replacing a fragile regex), adds RSCReferenceDiscoveryPlugin public export, and tightens the webpack manifest to only include chunk groups with client-reference async blocks. Well-motivated, clearly documented, solid test coverage. Key concerns (see inline comments): (1) BOM coverage gap in clientReferences.ts -- old code stripped BOM explicitly, new Acorn path handles it transparently but has no test; (2) jest.spyOn(acorn, parse) fragility in client-references.test.ts -- relies on CommonJS module-instance sharing, breaks silently under ESM, prefer a real parse error trigger; (3) charCodeAt magic numbers in clientReferences.ts -- inline char-name comments would help; (4) dep.type string fallback scope in react-server-dom-webpack-plugin.js needs a clarifying comment; (5) universal cacheable(false) cost in RSCReferenceDiscoveryPlugin.ts is correct but should be documented in plugin option docs. No correctness bugs found -- all concerns are hardening improvements, not blockers. |
2 participants
Important
Replacement / maintainer-owned follow-up for #46. This is paired with shakacode/react_on_rails#3556 and should be reviewed together with that PR.
Summary
RSCReferenceDiscoveryPluginpackage export and loader recording path from Derive RSC client refs from the RSC graph #46.'use client'; export const x = 1.compiler.webpacknorcompiler.rspackis available instead of falling through to a barerequire('webpack').as nevercasts in manifest tests by typing the mock client-reference shape explicitly.Verification
tests/client-references.test.tsfailed for same-line'use client'detection.tests/rsc-reference-discovery-plugin.test.tsfailed for the missing bundler API diagnostic.tests/react-flight-webpack-plugin-client-reference-chunks.test.tsfailed for the missing warning on chunk groups without block metadata.yarn jest tests/client-references.test.ts tests/rsc-reference-discovery-plugin.test.ts tests/react-flight-webpack-plugin-client-reference-chunks.test.ts tests/react-flight-webpack-plugin-css-order.test.ts --runInBandyarn testyarn buildgit diff --check/Users/justin/.agents/skills/autoreview/scripts/autoreview --mode localNotes
The existing
cacheable(false)behavior is intentionally preserved. #46's tests already assert that loader output is non-cacheable even for currently non-client modules, because otherwise a watch rebuild can miss a file that gains a'use client'directive.Note
Medium Risk
Changes build-time RSC client manifest chunk mapping and directive discovery; incorrect behavior could mis-wire client components at runtime, though behavior is heavily tested.
Overview
Adds graph-derived RSC client-reference discovery: a public
RSCReferenceDiscoveryPluginrecords"use client"modules during the RSC Webpack loader pass and emitsrsc-client-references.json, with loader output marked non-cacheable while the plugin is active so watch rebuilds do not miss newly added directives.hasUseClientDirectiveis centralized inclientReferences(Acorn-based parsing) and shared with Rspack; it now handles edge cases such as same-line directives and stricter prologue rules.ReactFlightWebpackPluginclient manifests tie modules to chunks using client-reference async blocks per chunk group (withgetBlocks/blocksIterablefallbacks), reducing unrelated entry chunks in the client manifest and emitting a Webpack warning when block metadata is missing instead of silently skipping.Reviewed by Cursor Bugbot for commit 5695cf9. Bugbot is set up for automated code reviews on this repo. Configure here.