Conditionally require react-dom/client in reactHydrate/reactRender if React version >= 18

[summera]

Summary

This resolves #1441.

With React 18, hydrateRoot and createRoot have been moved to react-dom/client. See reactwg/react-18#125 and facebook/react#23385 for more info.

If you try to access these methods on react-dom, you'll see a warning that looks like this:

Warning: You are importing createRoot from “react-dom” which is not supported. You should instead import it from “react-dom/client”.

React 18 support was added in #1409 but also resulted in this warning since createRoot and hydrateRoot were being called on react-dom.

In order to prevent the warning, I added a conditional require using a ternary statement in reactHydrate.ts and reactRender.ts. I also had to disable the import/no-unresolved rule as it was throwing an error when react-dom/client wasn't present (in React < 18). I think this should do it but I struggled making eslint and typescript happy so let me know if I'm missing anything.

---

This change is 

[Judahmeek]

@summera Thanks for your contribution!

Unfortunately, it's failing CI.

Looks like the require("react-dom/client") even though /spec/dummy/yarn.lock's version of r react-dom is v16.

I'm not sure what the solution is.

Maybe

const reactDomSrc = supportsReactCreateRoot ? "react-dom/client" : "react-dom";
require(reactDomSrc);

???

[summera]

@Judahmeek unfortunately doing that results in the following errors:

5:18 error Require statement not part of import statement @typescript-eslint/no-var-requires
5:18 error Calls to require() should use string literals import/no-dynamic-require

If the approach taken by @tomdracz in #1449 works, I suggest we close this out in favor of that PR.

[tomdracz]

@Judahmeek unfortunately doing that results in the following errors:

5:18 error Require statement not part of import statement @typescript-eslint/no-var-requires

5:18 error Calls to require() should use string literals import/no-dynamic-require

If the approach taken by @tomdracz in #1449 works, I suggest we close this out in favor of that PR.

Think mine might suffer from similar issues, although the warnings are perhaps ok to ignore.

With the way React made it quite hard to support migrations due to API changes, I think the only clean ways to do it are:

• require react 18 and make new create root API optional. If not using the new API, the app will behave as before. Will still emit annoying warnings though and it's a bit of sledgehammer approach.
• create "adapter" subpackages, one for React < 18 and one for React 18+. They would encapsulate the logic that's added in my PR. This will create additional installation step (adding correct package for your react version), but we could have clear React dependency specified in those rather than React in Rails core

[summera]

@Judahmeek would you mind kicking off the workflows again? I may have gotten this passing with my latest commit, but I'd like to verify.

[justin808]

I just updated with master. This might pass CI now.

Which should we favor? This or #1449? @tomdracz @summera

[tomdracz]

I think this one has better chance of working than mine. Using require rather than dynamic import is more likely to work without issues.

Looks like we are still getting some spec fails so need to investigate that

[justin808]

@summera do you want to finish this one up? I'd love to get it merged!

[tomdracz]

I'm at loss with this one. Found reference to template strings in require https://stackoverflow.com/questions/42797313/webpack-dynamic-module-loader-by-require/42804941#42804941

I can see in the error we're getting empty context error. This leads me to believe that either:

• package is not there
• we're looking it up in a wrong directory (more likely)

Need to dig into this and see what is the context returned by that dynamic require statement. Might bring us closer to figuring out what's wrong (maybe some webpack setting?). It's as if it's not looking up module inside the node_modules folder (which I THINK it should by default)

[justin808]

@tomdracz Please keep us posted. This is an important fix!

[alexeyr]

Need to dig into this and see what is the context returned by that dynamic require statement. Might bring us closer to figuring out what's wrong (maybe some webpack setting?). It's as if it's not looking up module inside the node_modules folder (which I THINK it should by default)

I tried to understand it yesterday too (and made my own attempt). The documentation is pretty unclear, but as far as I understood, Webpack requires a base directory for all dynamic imports/requires and indeed doesn't look in node_modules :(

[alexeyr-ci1]

#1460 has been merged instead.