Load react-dom correctly based on React version

[ahangarha]

Summary

This PR fixes the warning related to loading react-dom in React 18.

Pull Request checklist

• Add/update test to cover these changes
• Update documentation
• Update CHANGELOG file

Other Information

Closes #1258

[gregorbg]

The code comment pointing to webpack/webpack#339 (comment) specifies that:

This will succeed on runtime and only print a warning while compiling (instead of an error).

This statement is correct, i.e. our frontend code compiles without error, but the warning is displayed as a transparent overlay when running webpacker dev server. It is also constantly printed to our logs. Is there a more elegant way than shutting webpacker up via config? Why is it trying to even import react-dom/client anyways when we are clearly referencing "react-dom": "^16.14.0" in our package.json?

[ahangarha]

@gregorbg Do you mean you still get the warning message in v2.7?
I personally tested on two different setups and I didn't notice any warning.

[gregorbg]

I am specifically saying that we get the error only on v2.7
Reverting to 2.6 releases makes the warning disappear.

We are using React and React-DOM v16.14.0 and our code is at https://github.com/thewca/worldcubeassociation.org. To reproduce, literally just spin up the Docker container with docker-compose up.

[justin808]

Hi @gregorbg, yes, the problem is crazy annoying.

Here's the solution: https://github.com/shakacode/react_on_rails/blob/master/docs/guides/rails-webpacker-react-integration-options.md?plain=1#L59-L86

@ahangarha can please double check:

1. this documentation is also in react-rails
2. we have the same logic in react-rails as react_on_rails and thus it's definitely a harmless warning.

CC: @Judahmeek

[gregorbg]

Thanks for pointing me in the right direction! However, I can't help but wonder the following:

The webpacker error message points me to WARNING in ./node_modules/react_ujs/react_ujs/src/reactDomClient.js 16:21-48. Looking at the source code, this file contains the following check:

import ReactDOM from "react-dom"

const reactMajorVersion = ReactDOM.version?.split('.')[0] || 16

// TODO: once we require React 18, we can remove this and inline everything guarded by it.
const supportsRootApi = reactMajorVersion >= 18

let reactDomClient = ReactDOM

if (supportsRootApi) {
  // This will never throw an exception, but it's the way to tell Webpack the dependency is optional
  // https://github.com/webpack/webpack/issues/339#issuecomment-47739112
  // Unfortunately, it only converts the error to a warning.
  try {
    // eslint-disable-next-line global-require,import/no-unresolved
    reactDomClient = require('react-dom/client');
  } catch (e) {
    // We should never get here, but if we do, we'll just use the default ReactDOM
    // and live with the warning.
    reactDomClient = ReactDOM;
  }
}

export default reactDomClient

where the relevant line is reactDomClient = require('react-dom/client');. The import is guarded by an if-check that basically reads "is the ReactDOM major version greater than or equal to 18?". In our case, we are using React 16 meaning this should return false, meaning this if should never be executed.

I can certainly implement the fix that you mentioned (and pointed us to in thewca/worldcubeassociation.org#7904, thanks a lot! 😄) but why is this warning even being shown in the first place?

[ahangarha]

Thanks @gregorbg for your feedback.

[tf]

This change appears to break SSR. See #1276.