React 18 conditional support #1449
Conversation
| @@ -213,6 +211,7 @@ function unmount(el: Element): void { | |||
| const domNode = document.getElementById(domNodeId); | |||
| if(domNode === null){return;} | |||
| try { | |||
| // Might need updating? https://reactjs.org/blog/2022/03/08/react-18-upgrade-guide.html | |||
There was a problem hiding this comment.
unmountComponentAtNode is considered legacy https://reactjs.org/docs/react-dom.html#unmountcomponentatnode but would need to store references to root instead if we were to update
| toExport = legacyRootHandler; | ||
| } else { | ||
| toExport = modernRootHandler; | ||
| } |
There was a problem hiding this comment.
Struggling to find a nicer way to get this going, can't think of a way to do a dynamic import and then reexport
There was a problem hiding this comment.
Would be fairly trivial with top-level await though 🤔
|
|
||
| let toExport; | ||
|
|
||
| if (supportsReactCreateRoot) { |
There was a problem hiding this comment.
This could be good based not only on React version but also some configuration option so it's an opt-in
|
Also worth noting reactwg/react-18#125 (reply in thread) and multiple roots (which is what we're doing) being discouraged, although not a great info on "why" |
I also noticed this while working on #1448. @justin808 would switching to portals be an option for react_on_rails? |
Don't think portals are suitable here, would normally use them for some modals and overlays rather than full component trees. It doesn't seem like the docs discourage it anywhere, so apart from that one comment, there's no clear rationale and options from React team. Probably good to leave as is for now |
|
@Judahmeek @justin808 any of you able to trigger the tests to see the output? |
There was a problem hiding this comment.
Looks fantastic with a quick code review!
@tomdracz I like your coding style a lot!
Reviewed 8 of 9 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @tomdracz)
node_package/src/clientStartup.ts line 214 at r1 (raw file):
Previously, tomdracz (Tom Dracz) wrote…
unmountComponentAtNodeis considered legacy https://reactjs.org/docs/react-dom.html#unmountcomponentatnode but would need to store references to root instead if we were to update
Agree. It looks like there is more to do: reactwg/react-18#5.
Consequently, I'm not sure we should use the new createRoot API until we're confident that ReactOnRails is ready, as that is why the old behavior is available.
node_package/src/helpers/renderHelper.ts line 12 at r1 (raw file):
Previously, tomdracz (Tom Dracz) wrote…
Would be fairly trivial with top-level await though 🤔
Looks great so far!
node_package/src/helpers/renderHelper.ts line 8 at r2 (raw file):
Previously, tomdracz (Tom Dracz) wrote…
This could be good based not only on React version but also some configuration option so it's an opt-in
100% agree! Given what I wrote in the previous comment of us not being 100% sure that we're fully ready!
|
@tomdracz we've got CI fixed. Is this one ready to go? |
|
#1460 has been merged instead. |
Another stab at #1441
This change is