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.

Sign up for GitHub

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

Jump to bottom

React: Allow Storybook packages to use React 17.x #12908

Merged
merged 1 commit into from
Oct 26, 2020
Merged

React: Allow Storybook packages to use React 17.x #12908

merged 1 commit into from
Oct 26, 2020

Conversation

mrmckeb
Copy link
Member

@mrmckeb mrmckeb commented Oct 25, 2020 •
edited by shilman
Loading

Issue: #12408

What I did

This PR "unlocks" Storybook packages from the 16.x branch, meaning that during installation, npm/Yarn can dedupe and have only one React version installed.

How to test

After installation, listing dependencies should now only show one version of React installed.

# npm
npm ls react

# Yarn
yarn list --pattern react

@mrmckeb mrmckeb added bug dependencies react cra Prioritize create-react-app compatibility labels Oct 25, 2020
jimmyandrade
jimmyandrade approved these changes Oct 25, 2020
View reviewed changes
Copy link
Member

@jimmyandrade jimmyandrade left a comment

Choose a reason for hiding this comment

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

LGTM

@mrmckeb mrmckeb force-pushed the fix/relax-react-dependencies branch from cfbcf94 to 0fe4063 Compare October 25, 2020 14:04
@storybookjs storybookjs deleted a comment from github-actions bot Oct 25, 2020
@shilman shilman added this to the 6.1 core milestone Oct 25, 2020
@mrmckeb mrmckeb force-pushed the fix/relax-react-dependencies branch from 0fe4063 to 28b6001 Compare October 25, 2020 14:16
@mrmckeb mrmckeb force-pushed the fix/relax-react-dependencies branch from 28b6001 to 32c2ea9 Compare October 25, 2020 14:19
@shilman shilman changed the title fix: allow Storybook packages to use React 17.x React: Allow Storybook packages to use React 17.x Oct 25, 2020
@mrmckeb mrmckeb marked this pull request as ready for review October 25, 2020 14:56
shilman
shilman approved these changes Oct 26, 2020
View reviewed changes
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.

Great stuff @mrmckeb! Fingers crossed 🤞

@shilman shilman merged commit 8842f3a into next Oct 26, 2020
@shilman shilman deleted the fix/relax-react-dependencies branch October 26, 2020 15:26
@shilman shilman mentioned this pull request Oct 26, 2020
Closed
@Hypnosphi
Copy link
Member

deduping is an optimization, and we can't rely on it. After this change, it's quite possible that React 17 will be installed for those packages even if user app is on 16.

@Hypnosphi
Copy link
Member

Hypnosphi commented Oct 26, 2020 •
edited
Loading

I believe #11628 was a mistake
cc @ndelangen

BTW #11492 should no longer be an issue with NPM 7: https://github.com/npm/rfcs/blob/latest/implemented/0025-install-peer-deps.md

@shilman
Copy link
Member

shilman commented Oct 26, 2020

@Hypnosphi why do you think was #11628 a mistake?

@mrmckeb
Copy link
Member Author

mrmckeb commented Oct 27, 2020

Agreed @Hypnosphi, this is not something we can rely on. We need to fix this properly by moving to peer-dependencies again. We discussed this in-depth on a few calls, but we needed to get out a quick fix and then look at how to fix this long-term.

Also, this should no longer be an issue when everyone is on React 17+:
https://reactjs.org/blog/2020/10/20/react-v17.html#gradual-upgrades

@shilman shilman mentioned this pull request Oct 27, 2020
Closed
shilman added a commit that referenced this pull request Oct 27, 2020
@shilman
Allow Storybook packages to use React 17.x per #12908
5164f5e
@shilman shilman mentioned this pull request Oct 27, 2020
Closed
@Hypnosphi
Copy link
Member

@shilman react & react-dom should always be peer dependencies of packages that provide react components, to ensure that there's a single instance of those.

@mrmckeb please take a look at the example repository. It's not like you can use components from different versions in a single React tree. You just can nest one React tree inside another. Actually, it was possible before as well, but you could encounter issues with event propagation

@shilman shilman mentioned this pull request Oct 27, 2020
Closed
@vespakoen
Copy link

I think you had to do the same for react-dom, now I get a bunch of these:

➤ YN0060: │ @storybook/router@npm:6.1.0-alpha.31 provides react@npm:17.0.0 with version 17.0.0 which doesn't satisfy ^16.14.0 requested by react-dom@npm:16.14.0
➤ YN0060: │ @storybook/theming@npm:6.1.0-alpha.31 provides react@npm:17.0.0 with version 17.0.0 which doesn't satisfy ^16.14.0 requested by react-dom@npm:16.14.0
➤ YN0060: │ @storybook/api@npm:6.1.0-alpha.31 provides react@npm:17.0.0 with version 17.0.0 which doesn't satisfy ^16.14.0 requested by react-dom@npm:16.14.0
➤ YN0060: │ @storybook/addons@npm:6.1.0-alpha.31 provides react@npm:17.0.0 with version 17.0.0 which doesn't satisfy ^16.14.0 requested by react-dom@npm:16.14.0
➤ YN0060: │ @storybook/client-api@npm:6.1.0-alpha.31 provides react@npm:17.0.0 with version 17.0.0 which doesn't satisfy ^16.14.0 requested by react-dom@npm:16.14.0
➤ YN0060: │ @storybook/components@npm:6.1.0-alpha.31 provides react@npm:17.0.0 with version 17.0.0 which doesn't satisfy ^16.14.0 requested by react-dom@npm:16.14.0
➤ YN0060: │ @storybook/ui@npm:6.1.0-alpha.31 provides react@npm:17.0.0 with version 17.0.0 which doesn't satisfy ^16.14.0 requested by react-dom@npm:16.14.0
➤ YN0060: │ @storybook/core@npm:6.1.0-alpha.31 provides react@npm:17.0.0 with version 17.0.0 which doesn't satisfy ^16.14.0 requested by react-dom@npm:16.14.0
➤ YN0060: │ @storybook/source-loader@npm:6.1.0-alpha.31 provides react@npm:17.0.0 with version 17.0.0 which doesn't satisfy ^16.14.0 requested by react-dom@npm:16.14.0
➤ YN0060: │ @storybook/addon-docs@npm:6.1.0-alpha.31 [11be4] provides react@npm:17.0.0 with version 17.0.0 which doesn't satisfy ^16.14.0 requested by react-dom@npm:16.14.0
➤ YN0060: │ @storybook/addon-actions@npm:6.1.0-alpha.31 provides react@npm:17.0.0 with version 17.0.0 which doesn't satisfy ^16.14.0 requested by react-dom@npm:16.14.0
➤ YN0060: │ @storybook/addon-backgrounds@npm:6.1.0-alpha.31 provides react@npm:17.0.0 with version 17.0.0 which doesn't satisfy ^16.14.0 requested by react-dom@npm:16.14.0
➤ YN0060: │ @storybook/addon-controls@npm:6.1.0-alpha.31 provides react@npm:17.0.0 with version 17.0.0 which doesn't satisfy ^16.14.0 requested by react-dom@npm:16.14.0
➤ YN0060: │ @storybook/addon-toolbars@npm:6.1.0-alpha.31 provides react@npm:17.0.0 with version 17.0.0 which doesn't satisfy ^16.14.0 requested by react-dom@npm:16.14.0
➤ YN0060: │ @storybook/addon-viewport@npm:6.1.0-alpha.31 provides react@npm:17.0.0 with version 17.0.0 which doesn't satisfy ^16.14.0 requested by react-dom@npm:16.14.0
➤ YN0060: │ @storybook/addon-essentials@npm:6.1.0-alpha.31 [11be4] provides react@npm:17.0.0 with version 17.0.0 which doesn't satisfy ^16.14.0 requested by react-dom@npm:16.14.0

@shilman
Copy link
Member

shilman commented Oct 28, 2020

Argh. Good call @vespakoen 🤦

@shilman shilman mentioned this pull request Oct 28, 2020
Merged
@ndelangen
Copy link
Member

ok, @shilman so this allows for react 17, but does not yet change the monorepo to it. (no lockfile changes)

@gaetanmaisse gaetanmaisse mentioned this pull request Nov 1, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shilman shilman shilman approved these changes

@jimmyandrade jimmyandrade jimmyandrade approved these changes

@ndelangen ndelangen Awaiting requested review from ndelangen

@alexandrebodin alexandrebodin Awaiting requested review from alexandrebodin

@alterx alterx Awaiting requested review from alterx

@CodeByAlex CodeByAlex Awaiting requested review from CodeByAlex

@igor-dv igor-dv Awaiting requested review from igor-dv

@jbovenschen jbovenschen Awaiting requested review from jbovenschen

@leonrodenburg leonrodenburg Awaiting requested review from leonrodenburg

@thani-sh thani-sh Awaiting requested review from thani-sh

@renaudtertrais renaudtertrais Awaiting requested review from renaudtertrais

@saponifi3d saponifi3d Awaiting requested review from saponifi3d

@theinterned theinterned Awaiting requested review from theinterned

@thomasbertet thomasbertet Awaiting requested review from thomasbertet

@tmeasday tmeasday Awaiting requested review from tmeasday

@z4o4z z4o4z Awaiting requested review from z4o4z

Assignees
No one assigned
Labels
bug cra Prioritize create-react-app compatibility react
Projects
None yet
Milestone
6.1 core
Development

Successfully merging this pull request may close these issues.
6 participants
@mrmckeb @Hypnosphi @shilman @vespakoen @ndelangen @jimmyandrade