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

Limit react versions to >=15 #1613

Merged
merged 6 commits into from
Aug 11, 2017
Merged

Limit react versions to >=15 #1613

merged 6 commits into from
Aug 11, 2017

Conversation

danielduan
Copy link
Member

Issue:
I don't think our code base supports React ~0.14 anymore. It's been a while since 15 was released and lots has changed in React land. I think it's probably better for us maintenance wise to drop 14 as well.

Either that, or we should have some tests in place to make sure our code base doesn't regress on 14.

This might break support for alpha builds like 16.0.0-alpha.2. We could also document it somewhere in a README about deprecating support instead of in a peer dependency warning that everyone ignores.

Suggestions?

What I did

Seems like we don't support React 0.14 anymore out of the box. Issue came up in support slack and our CRA example app won't take 0.14 either.

How to test

Run CRA example app.

@danielduan danielduan requested review from ndelangen and shilman August 7, 2017 22:51
@Hypnosphi
Copy link
Member

This might break support for alpha builds like 16.0.0-alpha.2

I believe there is a way not to break that

@ndelangen
Copy link
Member

Anyone with a strong opinion on this? who wants to review / approve? @storybooks/team

@ndelangen ndelangen requested review from a team and removed request for ndelangen August 10, 2017 15:00
@ndelangen ndelangen assigned danielduan and Hypnosphi and unassigned danielduan Aug 10, 2017
@codecov
Copy link

codecov bot commented Aug 10, 2017

Codecov Report

Merging #1613 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1613      +/-   ##
==========================================
+ Coverage    21.3%   21.32%   +0.02%     
==========================================
  Files         244      244              
  Lines        5384     5378       -6     
  Branches      666      664       -2     
==========================================
  Hits         1147     1147              
+ Misses       3741     3737       -4     
+ Partials      496      494       -2
Impacted Files Coverage Δ
...-native/src/preview/components/OnDeviceUI/index.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8284bc7...016d647. Read the comment docs.

Copy link
Member

@Hypnosphi Hypnosphi left a comment

Choose a reason for hiding this comment

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

Looks like it should be >=15.0.0 || ^16.0.0-alpha.

Checked it on http://jubianchi.github.io/semver-check/

16.0.0-beta.5 satisfies constraint >=15.0.0 || ^16.0.0-alpha

@danielduan
Copy link
Member Author

should we implement this across the board in every package.json?

@ndelangen
Copy link
Member

With what @Hypnosphi mentions, if we do merge this... we need to explicitly update every time a beta/alpha/rc is released.

@Hypnosphi
Copy link
Member

Hypnosphi commented Aug 10, 2017

we need to explicitly update every time a beta/alpha/rc is released.

Only after we make sure we support it. And only for later main semver versions (major/minor/patch) than 16.0.0.

@danielduan
Copy link
Member Author

Wouldn't the peerDep just be a warning anyway? I think it will still use any version that's available but complain to you in the console.

@danielduan
Copy link
Member Author

I think one warning on the main react package is probably enough instead of adding it for all the addons. I don't think RN works on [email protected] so for the time being, * should be fine.

@Hypnosphi
Copy link
Member

LGTM

@Hypnosphi Hypnosphi merged commit 26d3a42 into master Aug 11, 2017
@ndelangen ndelangen deleted the danielduan-react-peer branch August 11, 2017 21:39
@shilman shilman changed the title Proposal: Limit react versions to >=15 Limit react versions to >=15 Aug 12, 2017
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.

3 participants