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

Remove upper bound on react-native peerDependency #1424

Merged
merged 3 commits into from Jul 22, 2017

Conversation

atticoos
Copy link
Member

@atticoos atticoos commented Jul 6, 2017

Issue: #1418

The goal is to allow new react-native versions without having to maintain an upper bounded version in Storybook.

What I did

Updates Storyboard's react-native module to allow any versions >=0.27.0, rather than keeping a capped upper-bound. As described in #1418 (comment), if a breaking change is later introduced the next version of storyboard can set the new minimum version. This removes the need to maintain an upper bound every time react-native rotates out a new release. I personally think it's okay to be a little less strict here.

Note: we cannot use ^0.27.0 since this is a prerelease, which will not be satisfied by semver

How to test

  • Create a new react-native project on a version later than 0.43.x
  • Locally link @storybook/react-native and observe a successful installation without an invalid peerDependency message

@codecov
Copy link

codecov bot commented Jul 6, 2017

Codecov Report

Merging #1424 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1424   +/-   ##
=======================================
  Coverage   14.54%   14.54%           
=======================================
  Files         202      202           
  Lines        4649     4649           
  Branches      504      500    -4     
=======================================
  Hits          676      676           
- Misses       3539     3551   +12     
+ Partials      434      422   -12
Impacted Files Coverage Δ
app/react/src/client/preview/render.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/api/configs/init_api.js 40.47% <0%> (ø) ⬆️
app/react/src/server/iframe.html.js 29.26% <0%> (ø) ⬆️
addons/info/src/components/Props.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/configs/handle_keyevents.js 33.33% <0%> (ø) ⬆️
lib/ui/src/modules/api/actions/api.js 49.42% <0%> (ø) ⬆️
app/react/src/demo/Welcome.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/routes.js 0% <0%> (ø) ⬆️
app/react/src/server/build.js 0% <0%> (ø) ⬆️
app/react/src/server/babel_config.js 44.82% <0%> (ø) ⬆️
... and 23 more

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 ba3b2a8...df7bc09. Read the comment docs.

Copy link
Member

@ndelangen ndelangen left a comment

Choose a reason for hiding this comment

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

I'm not sure

Copy link
Member

@Gongreg Gongreg left a comment

Choose a reason for hiding this comment

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

I don't think we should lock out on versions. Especially on 0.43 when it is already quite old.

@ndelangen
Copy link
Member

@tmeasday can you take a look at this perhaps?

@tmeasday tmeasday self-assigned this Jul 19, 2017
@tmeasday
Copy link
Member

I guess it comes down to how "solid" we want to be at the expense of being a bit slow. It's not difficult for someone to check each new version of RN and bump the range when we see that it works. But obviously the fact that it hasn't happened shows that no maintainer is particularly on top of this right now.

In any case I think most people ignore peer dependency warnings anyway so I suspect it's all a bit academic. I don't think npm uses them to decide which version to use or anything like that. So ultimately, it's about: for the subset of users that would be bothered by a warning about SB being incompatible with RN, is it better to be "loose" and hope for the best (not show the warning if we aren't sure we are compatible), or "tight" (show the warning if we aren't sure).

Thinking about it like that, I would suggest the latter. What do you all think?

@Rovack
Copy link

Rovack commented Jul 19, 2017

@tmeasday The concern is that for anyone still using shrinkwrap, peer dependency issues mean actual errors. In that sense, erring on the side of being too "tight" can be a very real problem as well.

@tmeasday
Copy link
Member

Ahh! Ok so I am wrong when I say users don't typically care about peer deps.

I'm OK with loosening it then, at least until someone who is actively using RN wants to take responsibility for bumping our versions.

@shilman shilman merged commit bb7ce26 into storybookjs:master Jul 22, 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.

None yet

7 participants