Remaining React 18 changes

[alexeyr-ci1]

• Currently, unmount in clientStartup.ts does

  react_on_rails/node_package/src/clientStartup.ts

  Line 216 in fa2fe25

  ReactDOM.unmountComponentAtNode(domNode);

  With the new Root API, it should be replaced by root.unmount(), which means we need to store rendered roots. The current approach will not work due to https://github.com/facebook/react/blob/2e0d86d22192ff0b13b71b4ad68fea46bf523ef6/packages/react-dom/src/client/ReactDOMLegacy.js#L391-L401:

  if (__DEV__) {
    const isModernRoot =
      isContainerMarkedAsRoot(container) &&
      container._reactRootContainer === undefined;
    if (isModernRoot) {
      console.error(
        'You are calling ReactDOM.unmountComponentAtNode() on a container that was previously ' +
          'passed to ReactDOMClient.createRoot(). This is not supported. Did you mean to call root.unmount()?',
      );
    }
  }

  We should add tests which cover this. (Fix unmounting in React 18 #1466)

• Clarify situation with multiple roots and if we should use createPortal instead. See the discussion starting with React 18 conditional support #1449 (comment). Also note

  identifierPrefix: optional prefix React uses for ids generated by React.useId. Useful to avoid conflicts when using multiple roots on the same page. Must be the same prefix used on the server.

  in https://reactjs.org/docs/react-dom-client.html#createroot (the same option is available for hydrateRoot).

• Update places in docs which still use ReactDOM.render.

• Look if we want any changes based on React Router v6.

[alexeyr-ci1]

@tomdracz Is there anything else we need to change for React 18 that I've missed?

[alexeyr-ci1]

@tomdracz And to fix point 1, do you know if it will be enough to store the roots on findContext() when rendering, or can we get different instances of window/global between reactOnRailsPageLoaded and reactOnRailsPageUnloaded?

[justin808]

@alexeyr-ci1, @tomdracz should I push a release?

[alexeyr-ci1]

@justin808 @pulkitkkr's #1455 is almost ready, and I think we'll want a release including it. So I suggest waiting until it's done and making the release after.

[tomdracz]

@tomdracz Is there anything else we need to change for React 18 that I've missed?

Think that covers it!

@tomdracz And to fix point 1, do you know if it will be enough to store the roots on findContext() when rendering, or can we get different instances of window/global between reactOnRailsPageLoaded and reactOnRailsPageUnloaded?

Not entirely sure on this one but I would expect window/global to be consistent between the two calls. No idea what happens if you bring Turbo to the mix though

[justin808]

@alexeyr-ci1 should I push a new version? or wait? Please check the changelog to see if it's current.

[alexeyr-ci1]

Not entirely sure on this one but I would expect window/global to be consistent between the two calls. No idea what happens if you bring Turbo to the mix though

It's actually only used with Turbo, we really need integration tests for it. Added #1468.

@tomdracz Actually, do you know if it should be called on non-Turbo page unload?

[alexeyr-ci1]

@alexeyr-ci1 should I push a new version? or wait? Please check the changelog to see if it's current.

I still think to wait for #1455, unless you want it ASAP. The changelog does need to be updated, but let me do the Popmenu PR first and I'll come back to it.

[alexeyr-ci1]

@justin808 Updated the changelog.

[tomdracz]

It's actually only used with Turbo, we really need integration tests for it. Added #1468.

@tomdracz Actually, do you know if it should be called on non-Turbo page unload?

I think there's no need. Non Turbo stuff will just reload the page so we should get fresh mounts on new page. Haven't dug into gem internals so that's just an educated guess

[justin808]

@alexeyr-ci1 Should I push a release with React 18 support? We'll also have #1455 hopefully.

[georgii-ivanov]

Any updates on release? Is it available in npm?

[justin808]

I'm pretty sure this was fixed a long time ago.