-
Notifications
You must be signed in to change notification settings - Fork 232
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
Support React 18 #655
Conversation
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?
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.
Yeah, running against multiple versions of React would be great, we do that in our library, I'll see what I can do today
run tests against all supported major react versions support both versions and fix ts
fix test matrix to choose appropriate react versions
package.json
Outdated
There was a problem hiding this comment.
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.
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 Report
@@ Coverage Diff @@ ## main #655 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 15 15 Lines 246 247 +1 Branches 34 34 ========================================= + Hits 246 247 +1
Continue to review full report at Codecov.
|
I think having a { legacyRoot: true }
option is a good idea if you want to include it in these changes
Sure, I'll try to find some time to add it this weekend
Also, feel free to push to my branch
Ignore root creation code from code covereage Introduced next react types for React.Root
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.
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.
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)
})
},
...
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
)?
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
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.
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
# Conflicts: # .github/workflows/validate.yml # package.json
✔️ 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
commented
Aug 23, 2021
Is this code released on npm anywhere? Just checking, no pressure.
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.
@snowystinger I saw you merged this to master. Is it getting released?
(Never mind. Misread status update.)
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
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.
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.
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?
@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.
Sure, happy to leave it open as well. I figured that was the outcome after reading through the other threads. Thanks for the help!
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
commented
Oct 21, 2022
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?
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.
Uh oh!
There was an error while loading. Please reload this page.
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-ce8cfe5650c6baabe7c7d381598ab9fa639f76febd230fc7e04befff5759d8e4R26async
I've no idea what to do about this one right nowact
may be problematic reactwg/react-18#23