feat: support @vitejs/plugin-react v6 as peer#525
feat: support @vitejs/plugin-react v6 as peer#525james-elicx merged 12 commits intocloudflare:mainfrom
Conversation
commit: |
|
/bigbonk review |
There was a problem hiding this comment.
Good change overall — moving @vitejs/plugin-react from a hard dependency to a peer dependency is the right approach for letting users choose between v5 and v6. The dynamic import pattern, duplicate detection, and test coverage are solid.
I have a few comments, mostly about edge cases and one potential test reliability issue. Nothing blocking.
|
Review submitted on PR #525. Here's a summary of the feedback: Overall: Good change. Moving Key comments:
|
|
All resolved |
|
/bigbonk re-review |
1 similar comment
|
/bigbonk re-review |
There was a problem hiding this comment.
Good change. Moving @vitejs/plugin-react to a peer dependency is the right call for supporting both v5 and v6 without forcing unnecessary installs. The dynamic import pattern, duplicate detection, testability injection for getMissingDeps, and the check.ts conditional output for App Router deps are all well done.
One remaining issue from the previous review that wasn't addressed, and a couple of minor items.
|
Review submitted. Here's a summary of my feedback on PR #525: Overall: Good change. Moving 3 comments posted:
|
|
All resolved |
|
/bigbonk re-review |
There was a problem hiding this comment.
Good change overall. Moving @vitejs/plugin-react to a peer dependency is the right architectural choice for supporting v5 and v6 side by side. The implementation is clean — dynamic import pattern, duplicate detection, testability injection in getMissingDeps, and the conditional App Router deps in check.ts are all well done. CI is green.
Missing dependency in examples
The examples under examples/ (app-router-cloudflare, pages-router-cloudflare, hackernews, etc.) don't list @vitejs/plugin-react as a dependency. Since it was removed from vinext's dependencies and moved to peerDependencies, it's no longer installed transitively for consumers.
I verified that createRequire from the example directory fails:
$ node -e "require('module').createRequire('.../examples/app-router-cloudflare/package.json').resolve('@vitejs/plugin-react')"
// throws MODULE_NOT_FOUND
The monorepo CI may still work because pnpm hoists from packages/vinext's devDeps, but anyone cloning a standalone example won't have it. Each example should add "@vitejs/plugin-react": "^5.1.4" to its dependencies.
Inline comments below on specific files.
|
Review submitted successfully. Here's a summary of the feedback I posted on PR #525: Overall: Good change. Moving Key feedback:
|
|
All resolved |
|
/bigbonk re-review |
There was a problem hiding this comment.
Good change. Moving @vitejs/plugin-react from a hard dependency to a peer dependency is the right architectural move for supporting both v5 and v6 without forcing unnecessary installs.
The implementation is clean and thorough:
- Dynamic import with
createRequireresolution correctly handles monorepo hoisting - Duplicate plugin detection mirrors the existing RSC pattern
.catch()wrapper on the promise gives a clear vinext-prefixed error- All examples updated with the explicit dependency
check.tsconditionally shows App Router deps (not just always listing them)deploy.tsconsistently uses_isResolvablenow (good cleanup of the old_findInNodeModulesfor MDX too)- Test coverage is solid — auto-injection, duplicate detection, init deps, deploy missing deps, check report formatting
Two minor non-blocking notes inline.
| }, | ||
| "peerDependencies": { | ||
| "@mdx-js/rollup": "^3.0.0", | ||
| "@vitejs/plugin-react": "^5.1.4 || ^6.0.0", |
There was a problem hiding this comment.
Non-blocking: @vitejs/plugin-react isn't listed in peerDependenciesMeta, so package managers will always warn when it's missing — even for users who intentionally pass react: false. This is a reasonable default since the vast majority of users need it, but worth being aware of if you get bug reports about spurious peer dep warnings from people using SWC or other JSX transforms.
| // Assumes @vitejs/plugin-react top-level plugin names continue to use | ||
| // the vite:react* prefix across supported versions. | ||
| const reactRootPlugins = config.plugins.filter( | ||
| (p: any) => p && typeof p.name === "string" && p.name.startsWith("vite:react"), |
There was a problem hiding this comment.
The comment on line 1464-1465 about plugin name prefixes is good. Worth noting that @vitejs/plugin-react-swc uses vite:react-swc as its name — so if v6 adopts a different transform backend, the prefix would change. The current approach is consistent with the RSC detection pattern (which checks for name === "rsc"), so it's fine as-is. Just a heads-up for when v6 actually ships.
|
Review submitted with approval. Here's a summary of my review: Approved — This is a well-executed change. The PR moves Key strengths:
Two non-blocking inline notes:
|
This allows users to choose whether to use v5 or v6. To avoid installing unnecessary dependencies.
This is a small braking change.