-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Update react-test-renderer version #17865
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit dfc3607. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
Test is failing with calls to a deprecated function 'isBatchingLegacy' in react-test-renderer |
I think this is due to having a mixture of different versions between different react packages. Looks like a lot of versions of react packages are on version 16 when installing these locally. I think this is because a bunch of dev dependencies depend on react 16, for example enzyme-adapter-preact. |
@cameron-martin You sound right. FYI, this line looks like where the error was thrown from. |
Hey @souppower thank you so much for this contribution! @ndelangen could you check this out? |
I'll look into this at the weekend if I have time. |
@@ -61,7 +61,7 @@ | |||
"jest-specific-snapshot": "^4.0.0", | |||
"preact-render-to-string": "^5.1.19", | |||
"pretty-format": "^26.6.2", | |||
"react-test-renderer": "^16.8.0 || ^17.0.0", |
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 think we need a devDependency on the version we're currently using in the monorepo; otherwise yarn might install a too new of a version if we regenerate the lockfile.
Could you add this?
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.
Looks like the unit test are failing because of the update, could you have a look?
https://app.circleci.com/pipelines/github/storybookjs/storybook/25536/workflows/5f9b2073-68d0-44cc-b528-962a226d74a3/jobs/378122
🙏
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.
@ndelangen Thanks for your feedback!
I think we need a devDependency on the version we're currently using in the monorepo; otherwise yarn might install a too new of a version if we regenerate the lockfile.
Could you add this?
Did you mean that I should move react-test-renderer
in dependencies
to devDependencies
?
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.
Yes, the unit test is failing because of these 'isBatchingLegacy' of undefined
errors.
Maybe I should first check what change has been made to react-test-renderer
changed since version 17?
https://github.com/facebook/react/commits/main/packages/react-test-renderer
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.
Looks like react-test-renderer 18 is not meant to be used with older react.
We probably cannot be able to upgrade react-test-renderer before upgrading react and react-dom in this repo first 🤔
6ac0a33
to
dfc3607
Compare
Let me close this old PR as I don't know how to get this to work 🙏 If anyone manages to get this to work, you can either open a new PR or reopen this PR. |
Issue: #17831 (comment)
What I did
I updated react-test-renderer to version 18.
The version we had depended on react 17, but this PR should fix the peerDeps problem.
https://github.com/facebook/react/blob/main/packages/react-test-renderer/package.json#L26-L28
@cameron-martin @jcerri Thanks for telling me about this issue! 🙌
How to test
If your answer is yes to any of these, please make sure to include it in your PR.