Use latest createRoot API

[maedi]

In React 18, createRoot and hydrateRoot are in react-dom/client. This PR uses this new API and provides backwards compatibility for React 16/17.

[justin808]

@maedi Looks good. Why is there an update to the yarn.lock but not package.json?

Is that change needed?

[justin808]

@maedi can you update the https://github.com/reactjs/react-rails/blob/master/CHANGELOG.md ?

We should link to PRs and authors like this: https://github.com/shakacode/shakapacker/blob/master/CHANGELOG.md

[maedi]

@justin808 Running bundle exec rake ujs:update only causes yarn.lock to update. In general I'm trying to stay away from package.json and the general build process of npx webpack && NODE_ENV=production npx webpack -p as I'm in the 'sub module' react_ujs, but I don't really understand the structure of this project. Does this workflow seem okay?

At the end of the day I'm not actually introducing any new packages or changing versions, I'm just handling multiple versions that come my way. Would you like the PR to include or remove yarn.lock? The sha's change and there are some version bumps in there.

[maedi]

I've tested this to work with React 17 but would always appreciate another tester.

[justin808]

@atout811, please pull the latest for https://github.com/atout811/rails-todo/pull/7

[FabioBachi]

@justin808 is this ready to be merged?
Thank you!

[justin808]

@FabioBachi, waiting to hear from @atout811.

[atout811]

@maedi I test on react 17 and it's failed and gives this error.

• I put your git repo link in Gemfile and Package.json
• bundle install & yarn install.
• run in terminal rails s.
• in another terminal ./bin/webpacker-dev-server.

the error was in ./bin/werbapcker-dev-server terminal.

and this is my PR where I test your code. PR7

[justin808]

@maedi can you let us know if you see any errors on React 16 or React 18? Can you confirm that you ran rm -rf node_modules && yarn when you switched branches?

[atout811]

Updates:

• To avoid this warning provided in the previous comment we put overlay: false in webpacker.yml and this warning is avoided in the browser but still in the terminal, and the app working correctly.

• Tried to use react 16: the warning still appears, but we avoided it like the previous point, after avoiding it's working well too.

• so we should document this warning to avoid confusion while using the package.

[justin808]

@maedi @atout811 I recommend that we use the same code that we used for React on Rails to detect the React Version:

https://github.com/shakacode/react_on_rails/pull/1460/files#diff-fb10f457d13849329ff53b16dc7953a9bc3a1d2468f3f0304398c6935e1f193fR6

const supportsReactCreateRoot = ReactDOM.version &&
  parseInt(ReactDOM.version.split('.')[0], 10) >= 18;

Here's the full file:

https://github.com/shakacode/react_on_rails/blob/master/node_package/src/reactHydrateOrRender.ts

import type { ReactElement } from 'react';
import ReactDOM from 'react-dom';
import type { RenderReturnType } from './types';

type HydrateOrRenderType = (domNode: Element, reactElement: ReactElement) => RenderReturnType;
const supportsReactCreateRoot = ReactDOM.version &&
  parseInt(ReactDOM.version.split('.')[0], 10) >= 18;

// TODO: once React dependency is updated to >= 18, we can remove this and just
// import ReactDOM from 'react-dom/client';
// eslint-disable-next-line @typescript-eslint/no-explicit-any
let reactDomClient: any;
if (supportsReactCreateRoot) {
  // 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 const reactHydrate: HydrateOrRenderType = supportsReactCreateRoot ?
  reactDomClient.hydrateRoot :
  (domNode, reactElement) => ReactDOM.hydrate(reactElement, domNode);

export function reactRender(domNode: Element, reactElement: ReactElement): RenderReturnType {
  if (supportsReactCreateRoot) {
    const root = reactDomClient.createRoot(domNode);
    root.render(reactElement);
    return root;
  }

  // eslint-disable-next-line react/no-render-return-value
  return ReactDOM.render(reactElement, domNode);
}

export default function reactHydrateOrRender(domNode: Element, reactElement: ReactElement, hydrate: boolean): RenderReturnType {
  return hydrate ? reactHydrate(domNode, reactElement) : reactRender(domNode, reactElement);
}

[justin808]

@maedi do you want to update this code? If not, I can find a ShakaCode team member to apply the same technique.

If you see the prior testing comments, we'll avoid needing to add docs as described here: #1194 (comment)

@BookOfGreg any suggestions?

[maedi]

@justin808 Back on this, I can update the code

[justin808]

@maedi any update?

[justin808]

@maedi can you confirm the code works with React 16, 17, and 18 in this repo: https://github.com/atout811/rails-todo, with HMR? with no warnings?

[maedi]

@justin808 Are you able to take this ticket over? I haven't tested on React 16.

[justin808]

@maedi how confident are you in the fix? What have you done to test the fix?

[JohnSmall]

When does this fix appear in the latest version? I see the warning after installing react-rails 2.6.2 on 2022-09-24.

react-dom.development.js:84 Warning: You are importing createRoot from "react-dom" which is not supported. You should instead import it from "react-dom/client".

[justin808]

@maedi any update?

@JohnSmall any chance you can put a set of eyes on this PR?

@BookOfGreg What is the process to push a beta release?

[kvognar]

It seems to work for me on React 18, but I am getting the same warning as JohnSmall!

[hibachrach]

I think this may prove problematic in the indefinite future. We're trying to support multiple major versions of a peer dependency react-dom, but conditional build-time imports only supported in certain environments (e.g. webpack). I think we would be better served by pushing react-dom to a peerDependency which assumes v18 but accepts an API for overriding how to call it.

[alkesh26]

@justin808 Let's get this fixed from the ShakaCode team member.

[justin808]

Closing for now. We'll look at React 18 support once 2.7 ships.

[ahangarha]

@justin808 Should I move this to the v3 milestone?

[justin808]

@ahangarha is this still an issue?

[ahangarha]

Well with #1269, this issue is fully resolved.