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

Move @storybook/addons to peerDependencies in all addons #2335

Merged
merged 8 commits into from
Nov 28, 2017

Conversation

Hypnosphi
Copy link
Member

Issue: #1981 #1892
We need a single instance of @storybook/addons to be shared between addons and app.

What I did

I moved the package to peerDeps in all the addons (some of them already had it there). There's no way to use addons without an app, so users still don’t have to install addons package on their side. Note that this relies on npm flattening the dependencies (the addons package is kept as a direct dep in apps)

A better solution would be to make the package a peerDep everywhere including the app, but that would be a breaking change, as users would have to install the package on their side.

How to test

I actually can’t find a good way to test that without publishing a new alpha version =(

@Hypnosphi Hypnosphi added this to the v3.3.0 milestone Nov 17, 2017
@codecov
Copy link

codecov bot commented Nov 17, 2017

Codecov Report

❗ No coverage uploaded for pull request base (release/3.3@5f6cef2). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff               @@
##             release/3.3    #2335   +/-   ##
==============================================
  Coverage               ?   21.58%           
==============================================
  Files                  ?      339           
  Lines                  ?     7108           
  Branches               ?      871           
==============================================
  Hits                   ?     1534           
  Misses                 ?     4924           
  Partials               ?      650

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 5f6cef2...f78732a. Read the comment docs.

@@ -14,7 +14,6 @@
"storybook": "start-storybook -p 9010"
},
"dependencies": {
"@storybook/addons": "^3.3.0-alpha.3",
Copy link
Member Author

Choose a reason for hiding this comment

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

It isn't actually used here

Copy link

@TroySchmidt TroySchmidt left a comment

Choose a reason for hiding this comment

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

Get it up to date with the latest alpha release all the way thru to push this out.

@Hypnosphi Hypnosphi requested a review from a team November 21, 2017 23:07
@@ -33,6 +32,7 @@
"shelljs": "^0.7.8"
},
"peerDependencies": {
"@storybook/addons": "^3.3.0-alpha",
Copy link
Member

Choose a reason for hiding this comment

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

should this be ^3.3.0-alpha.3 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It depends on whether we want to allow different versions of our packages to be used. If not, we should omit the caret in all cross-dependencies (direct and peer)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, it turns out that lerna can update peerDeps as well, so I changed the version to the current one everywhere

# Conflicts:
#	addons/a11y/package.json
#	addons/actions/package.json
#	addons/events/package.json
#	addons/info/package.json
#	addons/knobs/package.json
#	addons/links/package.json
#	addons/notes/package.json
#	addons/options/package.json
#	addons/storyshots/package.json
@Hypnosphi Hypnosphi requested a review from a team November 24, 2017 22:04
@Hypnosphi Hypnosphi self-assigned this Nov 28, 2017
@Hypnosphi Hypnosphi merged commit 57d8295 into release/3.3 Nov 28, 2017
@Hypnosphi Hypnosphi deleted the addons-peer branch November 28, 2017 22:59
@TroySchmidt
Copy link

Cannot wait to try this out!

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