version on react + run prettier #537
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
|
|
So found that the errors in react 16 & 17 were of using createRoot (as it's not available in those versions). Tested with all the versions, react 16, 17 and 18 with a vite project, by replacing the node_modules/million/dist with the locally built one (by pnpm build) |
|
Do I need to do anymore changes? |
|
good work! |
| : (parent[REACT_ROOT] = createRoot(parent)); | ||
| root.render(vnode); | ||
| if (version.startsWith('18')) { | ||
| import('react-dom/client') |
There was a problem hiding this comment.
This might break Webpack which will attempt to import react-dom/client even if using react 17 because bundlers are not smart enough to evaluate version.startsWith('18').
I tested with and got the following error on https://github.com/getsentry/sentry (which uses React 17).
ERROR in ../node_modules/million/dist/chunks/utils.mjs 46:6-32
Module not found: Error: Can't resolve 'react-dom/client' in '/Users/abhijeetprasad/workspace/sentry/node_modules/million/dist/chunks'
resolve 'react-dom/client' in '/Users/abhijeetprasad/workspace/sentry/node_modules/million/dist/chunks'
Parsed request is a module
using description file: /Users/abhijeetprasad/workspace/sentry/node_modules/million/package.json (relative path: ./dist/chunks)
IMO the more holistic solution is to actually not do this built time - but instead split up the renderReactScope function into renderReactScopev17andbelow and renderReactScope and inject different one's depending on what version of react you're using, so we take care of this at built time instead of run time.
There was a problem hiding this comment.
I also tried adding million to https://github.com/romansndlr/react-vite-realworld-example-app with this patch (vite app using React 17) and things broke during build as well.
[vite]: Rollup failed to resolve import "react-dom/client" from ".yalc/million/dist/chunks/utils.mjs".
This is most likely unintended because it can break your application at runtime.
If you do want to externalize this module explicitly add it to
`build.rollupOptions.external`
error during build:
Error: [vite]: Rollup failed to resolve import "react-dom/client" from ".yalc/million/dist/chunks/utils.mjs".
This is most likely unintended because it can break your application at runtime.
.yalc comes from https://github.com/wclr/yalc - which is what I used to test million locally.
There was a problem hiding this comment.
I think million new version containing this fix is yet to be released. Can you try replacing the node_modules/million/dist folder with the node_modules/million/dist folder from this PR's build?
There was a problem hiding this comment.
Might be worthwhile to also test Next 12 - which many folks are still using because they can't upgrade to Next 13 yet, and Next 12 uses React 17 by default.
Next.js and it's webpack setup might also cause issues here.
There was a problem hiding this comment.
I think million new version containing this fix is yet to be released. Can you try replacing the dist folder with the dist folder from this?
I'm using the most recent version on master branch! I built the repo locally and directly imported it into my project via yalc.
There was a problem hiding this comment.
Also, appreciate the feedback and you reaching out!
fix #534
/claim #534
Please describe the changes this PR makes and why it should be merged:
Status
Semantic versioning classification: