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

fix: Resolve warning using the wrong react-dom entrypoint #1018

Merged
merged 3 commits into from
Mar 4, 2022

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Mar 1, 2022

BREAKING CHANGE: Older versions of React (< 18.0.0-rc.1) are no longer supported.

Working group post: reactwg/react-18#125
Tested on mui/material-ui@45e47e0 (#31263): https://app.circleci.com/pipelines/github/mui/material-ui/65789/workflows/b001664a-2d8f-4f70-b80b-f7366b2d11bf

What:

Use new client entrypoint of react-dom. We would need a separate entry point if we would want to support older versions of React.

Why:

Avoids warnings when using createRoot from react-dom
The current alpha of React Testing Library doesn't contain any new features compared to the current stable other than support for React 18. So the only reason to upgrade is if you want to use React 18 at which point we might as well drop support for older version.

How:

Only access createRoot and hydrateRoot from react-dom/client

Checklist:

  • [ ] Documentation added to the
    docs site
  • Tests
  • [ ] TypeScript definitions updated
  • Ready to be merged

@eps1lon eps1lon added the bug Something isn't working label Mar 1, 2022
@eps1lon eps1lon force-pushed the fix/client-entry-point branch 4 times, most recently from 66d031d to c2bd65d Compare March 1, 2022 18:21
@eps1lon eps1lon mentioned this pull request Mar 1, 2022
1 task
@eps1lon eps1lon changed the title fix: Use new react-dom/client entrypoint fix: Drop support for React < 18.0.0-rc.1 Mar 1, 2022
@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 1, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit f8e434f:

Sandbox Source
React Configuration
react-testing-library-examples Configuration

@codecov
Copy link

codecov bot commented Mar 1, 2022

Codecov Report

Merging #1018 (f8e434f) into alpha (1ba40c9) will increase coverage by 9.48%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##            alpha     #1018      +/-   ##
===========================================
+ Coverage   90.51%   100.00%   +9.48%     
===========================================
  Files           4         4              
  Lines         232       169      -63     
  Branches       49        35      -14     
===========================================
- Hits          210       169      -41     
+ Misses         20         0      -20     
+ Partials        2         0       -2     
Flag Coverage Δ
current 100.00% <100.00%> (?)
experimental 100.00% <100.00%> (?)
latest ?
next 100.00% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/act-compat.js 100.00% <100.00%> (ø)
src/pure.js 100.00% <100.00%> (+23.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ba40c9...f8e434f. Read the comment docs.

"rimraf": "^3.0.2",
"typescript": "^4.1.2"
},
"peerDependencies": {
"react": "*",
"react-dom": "*"
"react": "18.0.0-rc.1",
Copy link
Member Author

Choose a reason for hiding this comment

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

Caret range would match 18.0.0-rc.0-next which is definitely not what we want.
Will widen to ^18.0.0 once React 18 lands.

@eps1lon eps1lon requested a review from kentcdodds March 1, 2022 20:57
@eps1lon eps1lon marked this pull request as ready for review March 1, 2022 21:19
@alexkrolick
Copy link
Collaborator

So the only reason to upgrade is if you want to use React 18 at which point we might as well drop support for older version.

Well... for now. Later there will be features or bugfixes added on top of this release series. I'd lean towards adding an alternate entry point or flag.

@kentcdodds
Copy link
Member

I'm ok with this on the condition that:

  1. We do not make the latest official release require unreleased software. So this remains in alpha until React v18 is officially released.
  2. We continue to provide bug fixes/features as needed to the latest official release until the React v18 release happens.

So basically: "Do whatever you want to with alpha, but don't release a latest that requires a release candidate of React"

@RLiriaVA

This comment was marked as spam.

@eps1lon
Copy link
Member Author

eps1lon commented Mar 2, 2022

We do not make the latest official release require unreleased software. So this remains in alpha until React v18 is officially released.

Definitely. The alpha branch will not land until after npm install react will install React 18 i.e. until React 18 is released as stable. That was always the goal.

We continue to provide bug fixes/features as needed to the latest official release until the React v18 release happens.

Sure thing.

React Testing Library is pretty mature. Last year was basically just type fixes (partially self inflicted pain) and JSDOC improvements. The actual features are all located in @testing-library/dom. So the only scenario that may involve a legacy branch is when @testing-library/dom releases a new semver major which we want to include.

But overall I don't feel that bad about the change anymore considering we can drop 500 LoC.

Well... for now. Later there will be features or bugfixes added on top of this release series.

Let's make this a "may" not a "will". We haven't released a fix or feature in over a year. All we've done in the past 1-2 years is bumping @testing-library/dom which we can always do from a "legacy" branch.

The whole idea of these renderer specific library is that they don't have many features. The bulk of the features is implemented in the host specific libraries (@testing-library/dom).

I'm going to check how separate entrypoints will look. But if React 18 lands tomorrow I prefer this approach in which we can always add suport for legacy React later.

@kentcdodds
Copy link
Member

kentcdodds commented Mar 2, 2022

@eps1lon, you and I are aligned 👍

@RLiriaVA

This comment was marked as spam.

@eps1lon
Copy link
Member Author

eps1lon commented Mar 3, 2022

Hm so branching for legacy and non-legacy root seems really heavy. We either have to copy 90% or create additional abstractions. This all seems like a maintenance nightmare. The current testing approach is already quite sketchy because we have to merge results to find missing coverage.

At the same time this PR makes testing a lot more straight forward while removing a whooping 400 LoC.

So I'd rather get this PR in and wait for feedback. And if we feel like supporting older versions it's probably easier to create two release lines.

@kentcdodds
Copy link
Member

That sounds fine to me 👍 I have no problem telling people they can continue using what has been a very stable version of the software if they can't upgrade to v18 yet (when it's released of course).

@eps1lon
Copy link
Member Author

eps1lon commented Mar 4, 2022

Ok let's try this in @alpha and see what people say (if at all).

@eps1lon eps1lon changed the title fix: Drop support for React < 18.0.0-rc.1 fix: Resolve warning using the wrong react-dom entrypoint Mar 4, 2022
@eps1lon eps1lon merged commit ba24a4b into testing-library:alpha Mar 4, 2022
@eps1lon eps1lon deleted the fix/client-entry-point branch March 4, 2022 21:06
@github-actions
Copy link

github-actions bot commented Mar 4, 2022

🎉 This PR is included in version 13.0.0-alpha.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released on @alpha
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants