Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

React: Add support for react18's new root API #17215

Conversation

valentinpalkovic
Copy link
Contributor

@valentinpalkovic valentinpalkovic commented Jan 12, 2022

Issue: #10543 #17831

Currently, it is not possible to enable the new React Root API, which was introduced in React 17/18 (ReactDOM.createRoot). To test/use the newest concurrent features from React, it is necessary to mount a component differently. Instead of mounting it via ReactDOM.render(reactElement, domElement) a root has to be created (const root = ReactDOM.createRoot). The new API provides new methods to render/unmount components.

What I did

The new React Root Api (ReactDOM.createRoot) allows using the
newest concurrent features of React 18. A feature flag was added to
enable/disable the new Root API via the storybook's react options.

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented Jan 12, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 40747e0. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@valentinpalkovic valentinpalkovic changed the title Add possibility of using new React Root Api Add the possibility to use the new React Root Api Jan 12, 2022
@valentinpalkovic valentinpalkovic changed the title Add the possibility to use the new React Root Api Add the possibility to use the new React Root Api (React 18) Jan 12, 2022
@brutus-law
Copy link

Definitely needing something like this to start upgrading to React 18 for Suspense on internal apps. Any timeline on when this is mergable?

@shilman
Copy link
Member

shilman commented Feb 9, 2022

Hoping to look in the next week. Thanks for your patience!

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed with @chantastic since my React18 knowledge is sorely lacking. We are big fans of this work, but have some concerns mostly about the complexity of the CallbackWrapper and also the potential for a memory leak with the nodes map. In addition some other minor nits.

Thanks so much for your patience so far and for bearing with us as we get this to a mergeable state together. Would love to hear your feedback on any of these points, since we are still learning this new feature.

app/react/src/client/preview/render.tsx Show resolved Hide resolved
app/react/src/client/preview/render.tsx Outdated Show resolved Hide resolved
docs/faq.md Outdated Show resolved Hide resolved
docs/faq.md Outdated Show resolved Hide resolved
app/react/types/index.ts Outdated Show resolved Hide resolved
app/react/src/client/preview/render.tsx Outdated Show resolved Hide resolved
app/react/src/client/preview/render.tsx Outdated Show resolved Hide resolved
docs/faq.md Outdated Show resolved Hide resolved
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An additional question after reviewing with @kylegach @ndelangen @yannbf 😄

app/react/src/client/preview/render.tsx Show resolved Hide resolved
@valentinpalkovic valentinpalkovic force-pushed the render-react-components-with-new-root-api branch from 2cd13d2 to 5e3f711 Compare February 23, 2022 14:41
app/react/types/index.ts Outdated Show resolved Hide resolved
@yannbf
Copy link
Member

yannbf commented Mar 25, 2022

Hey @valentinpalkovic this is such a great contribution! I hope you don't mind – I'll update this branch and fix the merge conflicts, and attempt to add an E2E test for the flags you added.

@IanVS
Copy link
Member

IanVS commented Apr 6, 2022

I have a hacky POC working for the vite-builder. I'll put up a PR for it later this evening, but at least it looks like it's a solvable problem.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@valentinpalkovic @ndelangen tremendous job on this!! 🎉

@shilman
Copy link
Member

shilman commented Apr 7, 2022

Also, this breaks the vite builder, but @IanVS is on top of this and we expect to get it fixed soon

@shilman shilman merged commit 06cdf8c into storybookjs:next Apr 7, 2022
@valentinpalkovic valentinpalkovic deleted the render-react-components-with-new-root-api branch April 7, 2022 05:14
@ndelangen ndelangen mentioned this pull request Apr 7, 2022
3 tasks
IanVS added a commit to storybookjs/builder-vite that referenced this pull request Apr 7, 2022
An [exciting PR](storybookjs/storybook#17215) has been merged and released in `6.5.0-alpha.58 ` that adds storybook support for React 18.  However, it currently breaks in the vite-builder test because of an `import()` of the new `react-dom/client`, which only exists in react 18.  Due to vitejs/vite#6007, we have to do a little bit of trickery to stop vite from erroring out.  The approach I'm taking here is:

1) Rewriting the import of `react-dom/client` to a dummy file if it can't be `require.resolve()`ed.  This fixes the problem in production builds.
2) Additionally, adding `react-dom/client` to the list of `optimizeDeps.exclude` if the framework is react and the package can't be required.  This is necessary to prevent vite from trying to pre-bundle, and throwing errors in dev.

This PR also updates the examples to the new `6.5.0-alpha.58` version of storybook, which includes the PR mentioned above, so that it can be tested in our react examples, and it adds a new `react-18` example as well.
@jonniebigodes
Copy link
Contributor

@valentinpalkovic, one final thing from you if you don't mind. If you could message me on Discord (same username), I'd appreciate it, I just want to follow up with you on an item related to this pull request.

@IanVS IanVS mentioned this pull request Apr 22, 2022
2 tasks
@nareshbhatia
Copy link

@valentinpalkovic, @shilman, there is a subtle issue in Valentin's repo that I can't put my finger on. It builds fine as is and both react app and storybook run fine. However if I delete package-lock.json and build from scratch then I get this error when trying to run storybook:

ModuleNotFoundError: Module not found: Error: Can't resolve '@mdx-js/react' in '/Users/naresh/projects/cra-with-storybook-valentinpalkovic/src/stories'
    at /Users/naresh/projects/cra-with-storybook-valentinpalkovic/node_modules/webpack/lib/Compilation.js:2016:28
    at /Users/naresh/projects/cra-with-storybook-valentinpalkovic/node_modules/webpack/lib/NormalModuleFactory.js:798:13
    at eval (eval at create (/Users/naresh/projects/cra-with-storybook-valentinpalkovic/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:10:1)
    at /Users/naresh/projects/cra-with-storybook-valentinpalkovic/node_modules/webpack/lib/NormalModuleFactory.js:270:22
    at eval (eval at create (/Users/naresh/projects/cra-with-storybook-valentinpalkovic/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:9:1)
    at /Users/naresh/projects/cra-with-storybook-valentinpalkovic/node_modules/webpack/lib/NormalModuleFactory.js:434:22
    at /Users/naresh/projects/cra-with-storybook-valentinpalkovic/node_modules/webpack/lib/NormalModuleFactory.js:116:11
    at /Users/naresh/projects/cra-with-storybook-valentinpalkovic/node_modules/webpack/lib/NormalModuleFactory.js:670:25
    at /Users/naresh/projects/cra-with-storybook-valentinpalkovic/node_modules/webpack/lib/NormalModuleFactory.js:855:8
    at /Users/naresh/projects/cra-with-storybook-valentinpalkovic/node_modules/webpack/lib/NormalModuleFactory.js:975:5
    at /Users/naresh/projects/cra-with-storybook-valentinpalkovic/node_modules/neo-async/async.js:6883:13
    at /Users/naresh/projects/cra-with-storybook-valentinpalkovic/node_modules/webpack/lib/NormalModuleFactory.js:958:45
    at finishWithoutResolve (/Users/naresh/projects/cra-with-storybook-valentinpalkovic/node_modules/enhanced-resolve/lib/Resolver.js:312:11)
    at /Users/naresh/projects/cra-with-storybook-valentinpalkovic/node_modules/enhanced-resolve/lib/Resolver.js:386:15
    at /Users/naresh/projects/cra-with-storybook-valentinpalkovic/node_modules/enhanced-resolve/lib/Resolver.js:435:5
    at eval (eval at create (/Users/naresh/projects/cra-with-storybook-valentinpalkovic/node_modules/enhanced-resolve/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:16:1)

I even tried to pin the version of storybook from "^6.5.0-alpha.64" to "6.5.0-alpha.64" but still can't get the build to run from scratch.

This is making it impossible to replicate Valentin's setup in my own repo where I performed the two steps as recommended and then substituting "^6.4.22" with "6.5.0-alpha.64". When I run storybook, I get the above error.

The other thing I tried was to replace "^6.5.0-alpha.64" with "next" so that npm picks up the new beta version (which it is doing), but still getting the same error as above.

Anything that I am missing?

@valentinpalkovic
Copy link
Contributor Author

valentinpalkovic commented Apr 25, 2022

@nareshbhatia Could you tell me which node and npm version you are using? I try to replicate it then. And could you provide the new package-lock.json file? Then I can take a closer comparison on both versions.

@nareshbhatia
Copy link

nareshbhatia commented Apr 25, 2022

Sure, @valentinpalkovic.

$ node -v
v16.13.2
$ npm -v
8.4.0

Here's the package.json with the storybook versions pinned.

{
  "name": "storybook-test-react-18",
  "version": "0.1.0",
  "private": true,
  "dependencies": {
    "@testing-library/jest-dom": "^5.16.4",
    "@testing-library/react": "^13.1.1",
    "@testing-library/user-event": "^13.5.0",
    "@types/jest": "^27.4.1",
    "@types/node": "^16.11.27",
    "@types/react": "^18.0.6",
    "@types/react-dom": "^18.0.2",
    "react": "^18.0.0",
    "react-dom": "^18.0.0",
    "react-scripts": "5.0.1",
    "typescript": "^4.6.3",
    "web-vitals": "^2.1.4"
  },
  "scripts": {
    "start": "react-scripts start",
    "build": "react-scripts build",
    "test": "react-scripts test",
    "eject": "react-scripts eject",
    "storybook": "start-storybook -p 6006",
    "build-storybook": "build-storybook"
  },
  "eslintConfig": {
    "extends": [
      "react-app",
      "react-app/jest"
    ]
  },
  "browserslist": {
    "production": [
      ">0.2%",
      "not dead",
      "not op_mini all"
    ],
    "development": [
      "last 1 chrome version",
      "last 1 firefox version",
      "last 1 safari version"
    ]
  },
  "devDependencies": {
    "@babel/core": "^7.17.9",
    "@storybook/addon-actions": "6.5.0-alpha.64",
    "@storybook/addon-essentials": "6.5.0-alpha.64",
    "@storybook/addon-interactions": "6.5.0-alpha.64",
    "@storybook/addon-links": "6.5.0-alpha.64",
    "@storybook/builder-webpack5": "6.5.0-alpha.64",
    "@storybook/manager-webpack5": "6.5.0-alpha.64",
    "@storybook/react": "6.5.0-alpha.64",
    "@storybook/testing-library": "^0.0.11",
    "babel-loader": "^8.2.5",
    "webpack": "^5.72.0"
  }
}

package-lock.json is attached.

@nareshbhatia
Copy link

package-lock.txt

Sorry, attachment did not work in last message. Here it is again.

@nareshbhatia
Copy link

Any update, @valentinpalkovic? Were you able to reproduce the issue?

@IanVS
Copy link
Member

IanVS commented Apr 28, 2022

I think this should be a new issue, instead of a conversation in a PR that a lot of people are subscribed to. Would you mind creating one, ideally with a link to a github repo that reproduces the problem?

@nareshbhatia
Copy link

I think this should be a new issue, instead of a conversation in a PR that a lot of people are subscribed to. Would you mind creating one, ideally with a link to a github repo that reproduces the problem?

New issue created: #18094

@ackvf
Copy link

ackvf commented Dec 21, 2022

How do I switch to the new createRoot api? I am still getting this error with storybok and storybook/react 6.5.15

@IanVS
Copy link
Member

IanVS commented Dec 22, 2022

@ackvf This is an old PR with a lot of people subscribed to it. Please feel free to join our Discord (discord.gg/storybook) to ask for help, or open an issue clearly describing the problem, if you think there is a bug and don't see an existing issue tracking it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet