Repository navigation
fix(rsc): allow JSX in .js client modules inside node_modules - #1295
Conversation
Many third-party Next.js client libraries ship plain `.js` files containing
`'use client'` (or `'use server'`) plus JSX. Next.js's SWC pipeline compiles
JSX in `.js` transparently, but vinext's `vinext:jsx-in-js` plugin
previously skipped node_modules entirely, so `@vitejs/plugin-rsc`'s
`rsc:use-client` analysis pass would feed the raw source to rolldown/oxc
with `lang: 'js'` and fail:
[plugin rsc:use-client] node_modules/.../client/index.js
RolldownError: Parse failed with 1 error:
Unexpected JSX expression
4: return <p>hello world</p>
This affected the Next.js deploy-suite test fixtures:
- test/e2e/app-dir/next-dist-client-esm-import
- test/e2e/app-dir/client-module-with-package-type
Fix: extend `vinext:jsx-in-js` to also handle `.js`/`.mjs` files inside
`node_modules` *when* they begin with a React directive. We mirror
`@vitejs/plugin-rsc`'s detection by parsing the directive prologue
(skipping BOM, hashbang, comments, and earlier directives like
`'use strict'`), which keeps the transform cost essentially zero for the
vast majority of node_modules files.
Validation:
- vp check (format / lint / type) — clean
- vp test run tests/jsx-in-js.test.ts tests/jsx-in-js-node-modules.test.ts
tests/intercepting-routes-build.test.ts tests/build-optimization.test.ts
- vp test run tests/nextjs-compat/ tests/font-google-build.test.ts
tests/build-time-classification-integration.test.ts
tests/tsconfig-path-alias-build.test.ts (281 passing, 0 regressions)
- vp run vinext#build (clean)
- New tests/jsx-in-js-node-modules.test.ts reproduces the reported error
on main and passes with the fix.
commit: |
|
|
/bigbonk review |
There was a problem hiding this comment.
Review: fix(rsc): allow JSX in .js client modules inside node_modules
Clean fix for a real production failure. The approach — scoping the JSX transform to directive-bearing node_modules .js files only — is the right design tradeoff between correctness and build performance. CI is all green.
What's good
-
Two-tier gating is smart. The cheap
code.includes("use client")bail-out avoids any work for the vast majority ofnode_modules.jsfiles. ThehasReactDirective()parser only runs when the substring is present, which correctly avoids false positives from incidental occurrences in comments or template literals. -
Directive prologue parser is correct for all practical inputs. I traced every code path: BOM stripping, hashbang, line/block comments, non-matching directives like
"use strict"before the React directive, and backtick/escaped-quote rejection. All behave correctly. The\r\nhandling is technically sloppy (post-directive skip on line 476 doesn't consume\r), but the outer/\s/whitespace loop on line 448 catches it. Since npm packages universally use\n, this is fine. -
Integration test reproduces the exact failure mode from the deploy-suite log and mirrors the Next.js fixture (
@monorepo/adapter-next) byte-for-byte. Good. -
Comment quality is excellent. The block comment explaining the plugin (lines 867–882) and the JSDoc on
hasReactDirectiveexplain the "why" clearly.
Suggestion: extract hasReactDirective and add unit tests
The function has ~45 lines of non-trivial parsing logic (BOM, hashbang, comments, directive scanning) with zero direct unit tests. The integration test exercises exactly one happy path ('use client' + JSX in .js). Consider extracting it to a utility module and adding a focused test file covering:
"use server"directive (half the target directives are untested)"use strict"preceding"use client"(the directive-scanning loop)"use client"in a comment or template literal (false positive prevention — the whole point of havinghasReactDirectiveinstead of justincludes).mjsfile (the regex matches both.jsand.mjsbut only.jsis tested)- BOM and hashbang prefixes
- Double-quoted
"use client"(the integration test only uses single quotes)
This would be fast to run (pure function, no Vite build) and would make the integration test a complementary E2E validation rather than the sole coverage.
Not blocking — the function is correct and the integration test validates the production failure is fixed. But given how many edge cases the parser handles, direct tests would catch future regressions much faster than a 90s build test.
| // Skip block comments. | ||
| if (code[i] === "/" && code[i + 1] === "*") { | ||
| const end = code.indexOf("*/", i + 2); | ||
| if (end === -1) return false; |
There was a problem hiding this comment.
Nit: the post-directive skip on line 475 consumes ;, space, and tab but not \r. For a file like "use strict"\r\n"use client", the \r is left dangling, and we rely on the outer /\s/ whitespace loop (line 448) to clean it up on the next iteration. This works correctly, but if you ever refactor this into a standalone module, consider normalizing the newline consumption here to:
while (i < len && (code[i] === ';' || /\s/.test(code[i]!))) i++;so the function is self-contained and doesn't depend on the outer loop's whitespace handling.
|
Review posted successfully. Here's a summary of what I reviewed: PR #1295: fix(rsc): allow JSX in .js client modules inside node_modules — Approved The PR fixes a production build failure where third-party Next.js libraries shipping Key findings from the review:
|
Summary
Many third-party Next.js client libraries ship plain
.jsfiles containing'use client'(or'use server') plus JSX. Next.js's SWC pipeline compiles JSX in.jstransparently, but vinext'svinext:jsx-in-jsplugin (added in #1199) explicitly skippednode_modules, so@vitejs/plugin-rsc'srsc:use-clientanalysis pass would feed the raw source straight to rolldown/oxc withlang: 'js'and crash the production build.Original error
Affected Next.js fixtures
test/e2e/app-dir/next-dist-client-esm-import— ships@monorepo/adapter-next/dist/client/index.jsas a plain.jsESM module that exports'use client'JSX.test/e2e/app-dir/client-module-with-package-type— similar pattern across ESM/CJS sub-packages (the JSX-bearing one is the failure mode this PR addresses).These are part of category A1 in the deploy-suite failure analysis.
Root cause
packages/vinext/src/index.tsconfiguresvinext:jsx-in-jswith:so any third-party
.jsfile containing JSX bypasses the JSX transform.@vitejs/plugin-rscthen parses that file with rolldown/oxc usinglang: 'js'(extension-inferred), which has no JSX support and throwsRolldownError: Unexpected JSX expression.Fix
Extend
vinext:jsx-in-jsso it also transforms.js/.mjsfiles insidenode_moduleswhen they begin with a React directive ('use client'or'use server'). A smallhasReactDirective()helper walks the directive prologue (skipping BOM, hashbang, line/block comments, and earlier directives like'use strict') so we only opt into the transform when there is a real directive —code.includes('use client')alone would match incidental occurrences in template literals or comments.Scoping the change this way:
rsc:use-clientparses are directive-bearing modules).node_modules.jsfiles, which exit the plugin after a cheapString#includescheck.lang: 'jsx'on arbitrary library code that may use syntax the JSX-enabled OXC parser does not accept.Validation
tests/jsx-in-js-node-modules.test.tsreproduces the exact error from the deploy-suite log onmain(fixture mirrors@monorepo/adapter-nextbyte-for-byte) and passes with the fix applied.