Support React 18

[snowystinger]

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
  

[snowystinger]

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?

[mpeyper]

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.

[snowystinger]

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

[snowystinger]

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.

[snowystinger]

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 })

[codecov]

Codecov Report

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

@@            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.

[mpeyper]

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

[snowystinger]

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

[mpeyper]

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.

[snowystinger]

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.

[snowystinger]

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)
      })
    },
...

[mpeyper]

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)?

[snowystinger]

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]

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.

[eps1lon]

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

[netlify]

✔️ 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]

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

[mpeyper]

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.

[gajus]

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

(Never mind. Misread status update.)

[snowystinger]

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

[mpeyper]

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.

[snowystinger]

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.

[snowystinger]

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?

[mpeyper]

@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.

[snowystinger]

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]

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.

[ralphstodomingo]

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?

[mpeyper]

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.