Require createRoot/hydrateRoot from 'react-dom/client' under react-dom 18+#1460
Conversation
alexeyr-ci1
force-pushed
the
alexeyr/conditional-react18-require
branch
from
e43626d to
68019ae
Compare
alexeyr-ci1
force-pushed
the
alexeyr/conditional-react18-require
branch
from
68019ae to
745b3f2
Compare
|
@tomdracz @summera Can you take a look at this one? Especially if you have actual React 18 apps to test it with? OTOH, some generated examples use React 18 and the warning doesn't seem to be in the logs https://github.com/shakacode/react_on_rails/runs/7024950174?check_suite_focus=true |
alexeyr-ci1
force-pushed
the
alexeyr/conditional-react18-require
branch
2 times, most recently
from
c705aa6 to
fcfbee9
Compare
|
@alexeyr LGTM! |
tomdracz
left a comment
tomdracz
left a comment
There was a problem hiding this comment.
LGTM, not entirely sure still why this one works where the other approaches didn't but 🤷♂️
Don't have React 18 app on hand to test though, if we have test cases that use it, it's good enough
| let reactDomClient: any; | ||
| if (supportsReactCreateRoot) { | ||
| // eslint-disable-next-line camelcase | ||
| if (__webpack_require__) { |
There was a problem hiding this comment.
Wonder if this is the magic thing that was missing in the other attempts!
There was a problem hiding this comment.
No, it isn't :) If I understand correctly (Webpack documentation is bad), #1448 was actually close to working, but Webpack needs a base directory for dynamic imports/requires. If it had
require(`react-dom/${supportsReactCreateRoot ? 'client' : 'index'}`);
it would recognize react-dom as the base directory and work.
There was a problem hiding this comment.
This if is needed because yarn run test in react_on_rails doesn't run under Webpack and require.context is not available.
There was a problem hiding this comment.
Don't have React 18 app on hand to test though, if we have test cases that use it, it's good enough
I've tried to update spec/dummy to React 18 and it doesn't work after all. Maybe the generated examples don't use Webpack?
There was a problem hiding this comment.
How about if we just use a different import for react 18 with react on rails?
Check source for react-rails?
Google to see what other libraries do which have react as a dependency?
There was a problem hiding this comment.
Some of the libraries I've seen that got direct dependency react-dom seem to have just released major versions that only support React 18 and left it at that. For all intents and purposes, React 18 components not initialized with the new root API should function the same as they do in React 17.
I do feel there's bit more at play here, think I've noted some other changes that might be required at one of the other PRs trying to tackle this.
alexeyr-ci1
force-pushed
the
alexeyr/conditional-react18-require
branch
from
c6d157f to
23d52e0
Compare
alexeyr-ci1
force-pushed
the
alexeyr/conditional-react18-require
branch
from
23d52e0 to
e7339ff
Compare
alexeyr-ci1
force-pushed
the
alexeyr/conditional-react18-require
branch
from
e7339ff to
61f7df8
Compare
|
@tomdracz @justin808 The actual solution turns out to be entirely different. Please review again. See #1463 for the React 18 test. |
|
|
| @ ./node_modules/react-on-rails/node_package/lib/ReactOnRails.js 34:45-78 | ||
| @ ./client/app/packs/client-bundle.js 5:0-42 32:0-23 35:0-21 59:0-26 | ||
| ``` | ||
| It can be safely [suppressed](https://webpack.js.org/configuration/other-options/#ignorewarnings) in your Webpack configuration. |
There was a problem hiding this comment.
I am concerning about version 16 or 18. I think there is a mistake there. I asked Alexey for clarification.
There was a problem hiding this comment.
Yes, I mentioned a fix for the phrasing.
There was a problem hiding this comment.
We could also put it into the config shakapacker exports. Would that potentially break anything? @justin808
6 participants
Fixes #1441
This change is