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

Storyshots: Allow react-test-renderer 18 #18296

Merged
merged 6 commits into from
Jul 11, 2022
Merged

Conversation

mvarendorff
Copy link
Contributor

Issue: addon-storyshots limits versions for react-test-renderer to 16/17 colliding with React 18 (see #17985)

What I did

Added ^18.0.0 to the allowed range for react-test-render so newer versions can be used without package.json hacks.

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 May 21, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit c917a20. 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.

@yannbf
Copy link
Member

yannbf commented May 23, 2022

Hey @geisterfurz007 thanks for your contribution! Could you update the lockfile as well? Thanks!

@mvarendorff
Copy link
Contributor Author

Yes of course, my bad! Should be better now 👍

@mvarendorff
Copy link
Contributor Author

@yannbf Hey, just to make sure; do you need anything else from me?

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

This looks like it's breaking CI. Storybook's monorepo is currently on React 17, so this causes the monorepo to use react-test-renderer 18, which is incompatible.

I wonder if we could get around this by adding the following to devDependencies to trick it to using v17 in the monorepo:

react-test-renderer": "^17.0.0"

WDYT? Mind giving it a try @geisterfurz007 ?

@mvarendorff
Copy link
Contributor Author

I hope I put it in the correct file; let me know if I didn't! I recall Yarn not being too happy with the same package in both dependency and devDependency so I assumed you meant the root package.json. 🤞

@shilman
Copy link
Member

shilman commented Jun 6, 2022

@geisterfurz007 that's not what I meant, but putting it at the root is pretty clever! let's see what CI says 😄

HMM no dice. I think we'll have to wait for upgrading the monorepo to react18 in v7 to revisit this, unless you have other ideas. Thanks for your patience!

@mvarendorff
Copy link
Contributor Author

mvarendorff commented Jun 6, 2022

@shilman Seems like moving the line up to the resolutions block works (at least the previously failing unit-tests now run through and the other failing circleci jobs appear to be caused by other packages).

Then again, there are also some resolved react-test-renderer@16 that are now forced up as well; I don't know if that causes any issues, but probably something that needs to be looked at?

Let me know if this counts as bandaid fix; otherwise I will stick to the resolutions field in my own project for the time being 😄

@shilman shilman changed the title Allow react-test-renderer 18 for storyshots (#17985) Storyshots: Allow react-test-renderer 18 Jul 11, 2022
@shilman shilman merged commit 3a8d6e2 into storybookjs:next Jul 11, 2022
@mvarendorff mvarendorff deleted the patch-1 branch July 11, 2022 18:33
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.

4 participants