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

feat: React 18 #7102

Closed
wants to merge 31 commits into from
Closed

feat: React 18 #7102

wants to merge 31 commits into from

Conversation

seyoon20087
Copy link
Contributor

@seyoon20087 seyoon20087 commented Apr 2, 2022

Breaking change

React version lower than 18 will now trigger an early error. Please make sure your site is using the new React version—you can declare dependency on React v18 specifically for Docusaurus, if you are in a monorepo with another version of React.

Motivation

React 18 is now officially released.

Unfortunately Docusaurus is still using legacy APIs on the client (as of now). This PR attempts to migrate off from legacy client APIs (like ReactDOM.hydrate to ReactDOMClient.hydrateRoot) by following the guide from https://reactjs.org/blog/2022/03/08/react-18-upgrade-guide.html. This should remove development warnings such as the one described in the React blog.

Legacy React versions would not continue to work due to the new ReactDOM rendering APIs. We've added a new early error.

Known Issue

Currently React throws a hydration error (#418 or #423) on the built HTML files (whether or not they are deployed). This is intended to exist in React 18 and should not break any part of the app, but may affect the Lighthouse score. In the meantime, you may ignore it. Related issue: facebook/react#24125

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Site runs

Related PRs

N/A

Signed-off-by: Shinwon Elizabeth Yoon <24852454+seyoon20087@users.noreply.github.com>
Signed-off-by: Shinwon Elizabeth Yoon <24852454+seyoon20087@users.noreply.github.com>
React 18 is now officially released (https://reactjs.org/blog/2022/03/29/react-v18.html).

Unfortunately Docusaurus is still using legacy APIs on the client.
Therefore I tried to migrate off from legacy APIs (like `ReactDOM.hydrate` to
`ReactDOMClient.hydrateRoot`) by following the guide from
<https://reactjs.org/blog/2022/03/08/react-18-upgrade-guide.html>.
Although they will be some users using legacy React versions. They will continue to work
as long as they use React >= 16.8 (which is the first release to support hooks).

KNOWN ISSUE

Currently opening the built HTML files on the browser might allow to React to throw a
hydration error(`#418`, `#423`). This is intended
(https://reactjs.org/blog/2022/03/08/react-18-upgrade-guide.html#other-breaking-changes) and
should not break any part of the app, but may affect the Lighthouse score. In the meantime, you
may ignore it.

Signed-off-by: Shinwon Elizabeth Yoon <24852454+seyoon20087@users.noreply.github.com>
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Apr 2, 2022
@netlify
Copy link

netlify bot commented Apr 2, 2022

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit 068703e
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/6252558971d84d0009c7b6fc
😎 Deploy Preview https://deploy-preview-7102--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented Apr 2, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 69
🟢 Accessibility 100
🟠 Best practices 83
🟢 SEO 100
🟢 PWA 90

Lighthouse ran on https://deploy-preview-7102--docusaurus-2.netlify.app/

Signed-off-by: Shinwon Elizabeth Yoon <24852454+seyoon20087@users.noreply.github.com>
Signed-off-by: Shinwon Elizabeth Yoon <24852454+seyoon20087@users.noreply.github.com>
Signed-off-by: Shinwon Elizabeth Yoon <24852454+seyoon20087@users.noreply.github.com>
Signed-off-by: Shinwon Elizabeth Yoon <24852454+seyoon20087@users.noreply.github.com>
Signed-off-by: Shinwon Elizabeth Yoon <24852454+seyoon20087@users.noreply.github.com>
@Josh-Cena Josh-Cena changed the title Add React 18 support feat: React 18 Apr 3, 2022
@Josh-Cena Josh-Cena added the pr: breaking change Existing sites may not build successfully in the new version. Description contains more details. label Apr 3, 2022
@Josh-Cena
Copy link
Collaborator

Josh-Cena commented Apr 3, 2022

NPM is unhappy with installing React 18 because MDX declares React 17 as peer dependency. I will wait for @slorber to decide what's the plan here—because that means if we don't upgrade to MDX v2, upgrading to React 18 is simply going to make every package manager slightly unhappy. Moreover, the console error is very, very disturbing.

@Josh-Cena Josh-Cena marked this pull request as draft April 3, 2022 07:24
@Josh-Cena Josh-Cena added the status: don't merge yet This PR is completed, but we are still waiting for other things to be ready. label Apr 3, 2022
This should resolve an issue related to peer dep check
enforced by `npm` package manager.

Signed-off-by: Shinwon Elizabeth Yoon <24852454+seyoon20087@users.noreply.github.com>
@Josh-Cena Josh-Cena added status: blocked This issue is blocked by another issue or external dep and can't be pushed further. and removed status: don't merge yet This PR is completed, but we are still waiting for other things to be ready. labels Apr 7, 2022
Signed-off-by: Shinwon Elizabeth Yoon <24852454+seyoon20087@users.noreply.github.com>
Signed-off-by: Shinwon Elizabeth Yoon <24852454+seyoon20087@users.noreply.github.com>
Signed-off-by: Shinwon Elizabeth Yoon <24852454+seyoon20087@users.noreply.github.com>
Signed-off-by: Shinwon Elizabeth Yoon <24852454+seyoon20087@users.noreply.github.com>
Signed-off-by: Shinwon Elizabeth Yoon <24852454+seyoon20087@users.noreply.github.com>
@Josh-Cena
Copy link
Collaborator

Josh-Cena commented Apr 8, 2022

@seyoon20087 Apologies if Sebastien didn't make it clear enough, but we'll probably not accept this PR because it will be quite a while before we have time to investigate proper migration to MDX v2 and hence React v18. We may keep this open but our PR backlog is already pretty bloated (currently 3:1 issue/PR ratio, which is outstanding among other OSSs), so it could be closed for housekeeping.

We require non-trivial PRs like this to have a matching issue, exactly because (a) you don't waste time implementing something we don't immediately need and (b) if the PR is rejected we can still keep it in backlog. Given the reasonings Sebastien has given above, could you first send a proposal about migration to React 18, similar to what we had for React 17, so we can investigate how to progressively roll it out, what dependencies are blocking this, what refactors we need to make, etc.?

@seyoon20087
Copy link
Contributor Author

... Sorry, we are not interested in maintaining a fork of MDX. Please refrain from making these architectural changes without first discussing with us. Thanks!

I am doing this because the original @mdx-js/react package has a peer dep of react 17 which causes this issue (related to this: vercel/next.js#35707)

@Josh-Cena
Copy link
Collaborator

Yes, I understand, but if it involves forking the entire dependency I'd rather wait and find a better solution :)

Signed-off-by: Shinwon Elizabeth Yoon <24852454+seyoon20087@users.noreply.github.com>
Signed-off-by: Shinwon Elizabeth Yoon <24852454+seyoon20087@users.noreply.github.com>
Signed-off-by: Shinwon Elizabeth Yoon <24852454+seyoon20087@users.noreply.github.com>
Signed-off-by: Shinwon Elizabeth Yoon <24852454+seyoon20087@users.noreply.github.com>
Signed-off-by: Shinwon Elizabeth Yoon <24852454+seyoon20087@users.noreply.github.com>
Signed-off-by: Shinwon Elizabeth Yoon <24852454+seyoon20087@users.noreply.github.com>
Signed-off-by: Shinwon Elizabeth Yoon <24852454+seyoon20087@users.noreply.github.com>
Signed-off-by: Shinwon Elizabeth Yoon <24852454+seyoon20087@users.noreply.github.com>
Signed-off-by: Shinwon Elizabeth Yoon <24852454+seyoon20087@users.noreply.github.com>
@seyoon20087 seyoon20087 marked this pull request as ready for review April 9, 2022 07:37
@seyoon20087 seyoon20087 marked this pull request as draft April 9, 2022 07:37
Signed-off-by: Shinwon Elizabeth Yoon <24852454+seyoon20087@users.noreply.github.com>
K0rdan added a commit to Luos-io/Documentation that referenced this pull request Apr 25, 2022
As stated here : facebook/docusaurus#7102 (review), we'll not follow React 18 updates as long docusaurus and their own dependencies are not update
@Josh-Cena Josh-Cena mentioned this pull request Apr 29, 2022
2 tasks
@Josh-Cena
Copy link
Collaborator

Opened #7264. @seyoon20087 Thank you very much for spending your time on this, but I don't think this PR will land any time soon, and it's a quite substantial change so it'll require roadmapping and design efforts from the team. I'll close the PR for now, but we'll eventually revisit the migration in the future when we have better vision of what we need to do and how we will do it.

In the future, for any substantial feature PRs (you have made quite a few, including strict mode, route announcer, etc.), please remember to first open an accompanying issue. It's mentioned in the contributing guideline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: breaking change Existing sites may not build successfully in the new version. Description contains more details. status: blocked This issue is blocked by another issue or external dep and can't be pushed further.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants