Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Support React 18 #655

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

Closed

Conversation

Copy link
Contributor

@snowystinger snowystinger commented Jul 21, 2021
edited
Loading

What:

Start a branch to support React 18

Why:

We use this library to test our hooks and we're looking to support React 18

How:

I've changed it over according to reactwg/react-18#5

Checklist:

There are definitely broken tests :) I'm just starting to look at things, if I'm not responsive enough, feel free to take the PR over.

areas I've identified as problematic:
dom rendering will throw two copies of an error https://github.com/testing-library/react-hooks-testing-library/pull/655/files#diff-ce8cfe5650c6baabe7c7d381598ab9fa639f76febd230fc7e04befff5759d8e4R26
async I've no idea what to do about this one right now
act may be problematic reactwg/react-18#23

  • Documentation updated
  • Tests
  • Ready to be merged
  • Added myself to contributors table

joshuaellis, brainkim, ellemedit, and kooku0 reacted with hooray emoji brainkim and kooku0 reacted with eyes emoji
Copy link
Contributor Author

On a side note, noticed this library makes it really hard to support multiple versions of React because of the lifecycle prepare which causes devDeps to be downloaded to people using the library's packages like a normal dependency
is there any way to get rid of that prepare and just create everything ahead of time?

Copy link
Member

mpeyper commented Jul 23, 2021
edited
Loading

You mean the npm script? I'm not fussed if you want to remove the prepare script as part of this if it helps.

I'm a bit short on time at the moment to look at this very deeply, but thanks for the effort you're putting in. I'll try to take a look over the weekend.

As a side note, I'd love it if we could run our tests against the current stable release and the next release of React on the CI pipeline.

Copy link
Contributor Author

Yeah, running against multiple versions of React would be great, we do that in our library, I'll see what I can do today

package.json Outdated
@@ -33,7 +33,6 @@
"scripts": {
"setup": "npm install && npm run validate -s",
"validate": "kcd-scripts validate",
"prepare": "npm run build",
Copy link
Contributor Author

@snowystinger snowystinger Jul 23, 2021

Choose a reason for hiding this comment

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

I'm not sure what else we'll need to do now that I've removed this because of https://docs.npmjs.com/cli/v7/commands/npm-install

If the package being installed contains a prepare script, its dependencies and devDependencies will be installed, and the prepare script will be run, before the package is packaged and installed.

Copy link
Contributor Author

Btw, not sure if you want to do something like this testing-library/react-testing-library#937
TLDR; use createRoot by default but allow for opt out render(element, { legacyRoot: true })

Copy link

codecov bot commented Jul 24, 2021
edited
Loading

Codecov Report

Merging #655 (07041f1) into main (21a20ba) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@ Coverage Diff @@
## main #655 +/- ##
=========================================
 Coverage 100.00% 100.00% 
=========================================
 Files 15 15 
 Lines 246 247 +1 
 Branches 34 34 
=========================================
+ Hits 246 247 +1 
Impacted Files Coverage Δ
src/dom/pure.ts 100.00% <100.00%> (ø)
src/server/pure.ts 100.00% <100.00%> (ø)

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 21a20ba...07041f1. Read the comment docs.

Copy link
Member

mpeyper commented Jul 24, 2021

I think having a { legacyRoot: true } option is a good idea if you want to include it in these changes

Copy link
Contributor Author

Sure, I'll try to find some time to add it this weekend
Also, feel free to push to my branch

mpeyper added 2 commits July 24, 2021 22:20
Ignore root creation code from code covereage
Introduced next react types for React.Root
Copy link
Member

mpeyper commented Jul 24, 2021

Re: the async tests timing out, it appears as though we are running into the same issue as described here by @eps1lon who has also made changes to their act functionality in the RTL PR. I'm not sure if there was ever a resolution to that discussion as it's still very unclear to me what we are supposed to do in this situation.

If act is removed from the async utils, the test pass again, but the console is littered with async act warnings, so that is not a suitable solution.

Copy link
Contributor Author

ah :( yeah I'd seen that comment but wasn't sure how it played a part yet. Thanks for clearing that up. Unfortunately, it would seem you need to be part of their working group to upvote the discussion. Thanks for the nice little refactor as well. I didn't have time on the weekend, but I'll add { legacyRoot: true } today.

Copy link
Contributor Author

Well, I didn't get to it today, this is the direction I was headed though.

createRoot.js

export function createRoot(container: Element) {
 let root
 let newRoot = {
 render: (hook, opts) => {
 root = (ReactDOM.createRoot && !(opts && opts.legacyRoot) ? ReactDOM.createRoot : createLegacyRoot)(container).render(hook)
 return newRoot
 },
 rerender: (hook) => {
 if (root) {
 return root.rerender(hook)
 }
 },
 unmount: () => {
 if (root) {
 return root.unmount()
 }
 },
 act: (args) => {
 if (root) {
 return root.act(args)
 }
 }
 };
 return newRoot
}
// similar for hydrate

dom/pure.js

...
 const root = createRoot(container)
 return {
 render(props?: TProps, opts?: {legacyRoot?: boolean}) {
 act(() => {
 root.render(testHarness(props), opts)
 })
 },
...

Copy link
Member

mpeyper commented Jul 30, 2021

I think I'd want the root to returned from the createRoot function to match the react 18 root interface.

As a side note, I don't think the act issue will have a resolution anytime soon, so would you like to put together a PR for your build changes to run against against versions of react we do currently support (16.9.0, ^16 and ^17)?

Copy link
Contributor Author

Sure, sorry for the delay, we've been preparing for a release and it's been taking most of my time this week. It does appear though that there will be a resolution somewhat soon. They seem to have a direction they are going to take reactwg/react-18#23 (reply in thread)
specifically, we'll just have to put up with the act warnings everywhere until they remove them :-/ but I'll break out the other stuff today and we can just leave 18 in this PR to wait for a bit longer

mpeyper reacted with thumbs up emoji

Copy link
Member

mpeyper commented Jul 30, 2021

Oh, thanks for pointing out that discussion had advanced.

With that in mind, we will probably need some kind of compat layer for calling act in the async utils that does the wrapping in react 16/17 but not in 18 as I don't want the warning appearing for existing users that haven't upgraded to react 18 yet.

I'm not sure on the implementation of that either as the way I understand those messages, act will still exist and it's just the situation to use it has changed. Also, is it only when using the concurrent root, or will we need to revert to react 17 behaviour of using the legacy root in react 18?

I really wish I was part of that working group because I've got some questions about when it will warn and when it won't.

brainkim reacted with thumbs up emoji

Copy link
Member

eps1lon commented Jul 30, 2021

I really wish I was part of that working group because I've got some questions about when it will warn and when it won't.

Feel free to post them via testing-library discord (and ping me). I can forward them

mpeyper reacted with heart emoji brainkim reacted with eyes emoji

# Conflicts:
#	.github/workflows/validate.yml
#	package.json
Copy link

netlify bot commented Aug 2, 2021
edited
Loading

✔️ Deploy Preview for react-hooks-testing-library ready!

🔨 Explore the source changes: 07041f1

🔍 Inspect the deploy log: https://app.netlify.com/sites/react-hooks-testing-library/deploys/61aeb332c8adfc000898af2d

😎 Browse the preview: https://deploy-preview-655--react-hooks-testing-library.netlify.app

brainkim reacted with eyes emoji

Copy link

Is this code released on npm anywhere? Just checking, no pressure.

Copy link
Member

mpeyper commented Aug 23, 2021

Hi @brainkim, no, not yet. If we can get something that passes the CI tests for all variations, then I'm happy to put out an alpha build.

It's been a while since I looked at this, but I think it was all the async tests that were timing out due to a change in the act implementation.

brainkim, snowystinger, and ellemedit reacted with heart emoji

Copy link

gajus commented Oct 26, 2021
edited
Loading

@snowystinger I saw you merged this to master. Is it getting released?

(Never mind. Misread status update.)

Copy link
Contributor Author

sorry for the confusion, i just like to keep branches fairly up to date over the long run, that way i don't have big conflicts potentially down the road

brainkim reacted with thumbs up emoji

Copy link
Member

mpeyper commented Oct 26, 2021

Thanks again for the effort @snowystinger. This is still something I definitely want to progress, I've just been very time poor recently to work on it myself, so any contributions are very welcome.

Copy link
Contributor Author

No worries, I've been out of touch with it as well. It would appear there has been progress on this front from React, the link earlier in this discussion #655 (comment) now points to reactwg/react-18#102 I'll try to find some time to see if it changes anything for this PR. Also, I won't be offended if someone else has time earlier than I do and opens their own PR.

Copy link
Contributor Author

hey, just catching up with all the discussions and specifically #688 (comment)
is there anything that would be good for me to tackle here? or should we close this PR?

Copy link
Member

mpeyper commented Dec 17, 2021

@snowystinger I'm happy to keep it open if anyone wants to tinker away at it, but it's looking like this library will be no more once the @testing-library/react and @testing-library/react-native have stable versions of renderHook available (context) which should occur before React 18 is officially released.

Copy link
Contributor Author

Sure, happy to leave it open as well. I figured that was the outcome after reading through the other threads. Thanks for the help!

mpeyper reacted with thumbs up emoji

Copy link
Member

mpeyper commented Apr 12, 2022
edited
Loading

I’m closing this now to prevent confusion about what we are doing with React 18.

Please see the note in the README for more context.

@snowystinger snowystinger deleted the react-18-upgrade branch April 12, 2022 22:07
Copy link

I must say, I read the discussion on react-testing-library and I would like to ask that you reconsider deprecating this package.

The experience I had trying to grok what differs between the renderHook implementations here and there, borderline horrible. I'm tearing my hair. Issues filed over there asking about the functionality missing e.g. waitFor- ones get only the reply that waitFor's going to supposedly solve the problems, but there are no concrete documented patterns to be found.

I'm even contemplating moving back to React 17 on a project just because of this - how is it considered acceptable these days to alienate a lot of library consumers by asking them to add much more boilerplate to do the same things that used to work before?

iamstarkov reacted with thumbs up emoji

Copy link
Member

mpeyper commented Oct 21, 2022

Hi @ralphstodomingo,

I’m so sorry it is causing you so much frustration. It’s not complete, but you can check out the work-in-progress migration guide I’ve been working on for far too long which may help get you over some humps.

Ultimately, a large part of the decisions made in the other thread was to avoid this situation in the future by moving to a common api with RTL. If you’ve read that issue than you would know that there is much about that decision that I disagree with, however, I also believe that having two competing renderHook apis is worse for the community in the long term than having a one time upgrade pain now. I know this will be of little comfort to you or others struggling with the migration now though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpeyper mpeyper mpeyper left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /