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

Update react-test-renderer version #17865

Closed

Conversation

souppower
Copy link
Contributor

@souppower souppower commented Apr 3, 2022

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

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented Apr 3, 2022

☁️ Nx Cloud Report

CI 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 target

Sent with 💌 from NxCloud.

@souppower
Copy link
Contributor Author

Test is failing with calls to a deprecated function 'isBatchingLegacy' in react-test-renderer

@cameron-martin
Copy link

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.

@souppower
Copy link
Contributor Author

@cameron-martin You sound right.

FYI, this line looks like where the error was thrown from.
ReactCurrentActQueue is undefined, so referencing ReactCurrentActQueue.isBatchingLegacy throws.
https://github.com/facebook/react/blob/fc47cb1b61ac012f9bd6d7251eb19fcecf364a3b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js#L790

@yannbf
Copy link
Member

yannbf commented Apr 11, 2022

Hey @souppower thank you so much for this contribution! @ndelangen could you check this out?

@souppower
Copy link
Contributor Author

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",
Copy link
Member

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?

Copy link
Member

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
🙏

Copy link
Contributor Author

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?

Copy link
Contributor Author

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.

image

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

Copy link
Contributor Author

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 🤔

yarn.lock Outdated Show resolved Hide resolved
@souppower
Copy link
Contributor Author

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.

@souppower souppower closed this Jun 13, 2022
@souppower souppower deleted the react-test-renderer-v18 branch June 13, 2022 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants