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

Enforce addons store being a singleton by storing it in global variable #3383

Merged
merged 1 commit into from
Apr 8, 2018

Conversation

Hypnosphi
Copy link
Member

@Hypnosphi Hypnosphi commented Apr 8, 2018

This should be the ultimate fix for #1981, hopefully

@codecov
Copy link

codecov bot commented Apr 8, 2018

Codecov Report

Merging #3383 into master will decrease coverage by 0.05%.
The diff coverage is 11.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3383      +/-   ##
==========================================
- Coverage   36.84%   36.78%   -0.06%     
==========================================
  Files         458      458              
  Lines       10032    10042      +10     
  Branches      900      909       +9     
==========================================
- Hits         3696     3694       -2     
- Misses       5773     5778       +5     
- Partials      563      570       +7
Impacted Files Coverage Δ
lib/addons/src/storybook-channel-mock.js 0% <ø> (ø)
lib/addons/src/index.js 0% <0%> (ø) ⬆️
addons/storyshots/src/index.js 85.71% <100%> (ø) ⬆️
addons/knobs/src/components/types/Array.js 34.14% <0%> (ø) ⬆️
lib/core/src/server/utils.js 40.47% <0%> (ø) ⬆️
addons/info/src/components/types/PropertyLabel.js 78.94% <0%> (ø) ⬆️
addons/jest/src/components/Indicator.js 0% <0%> (ø) ⬆️
addons/viewport/src/manager/components/Panel.js 37.26% <0%> (ø) ⬆️
addons/knobs/src/components/types/Boolean.js 11.62% <0%> (ø) ⬆️
... and 73 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 cdb8910...88c99a2. Read the comment docs.

@tmeasday
Copy link
Member

tmeasday commented Apr 8, 2018

I think this is good.

I want to register the downside of this approach (don't try and reconcile dependencies, just let the "first in" win), which is similar to using a require.alias to force a react version: If we ever release a new version of the addons API, we can't use the module install system to tell the user what's going on. (npm/yarn will happily install a variety of versions of @storybook/addons without even knowing this might not be what the user wants).

So in the future if we wanted to release @storybook/addons@2.0 we'd have to build our own adhoc dependency system: "You seem to have addons@1 and addons@2 installed, this is no good, please go search your node_modules".

Having said that, given that yarn/npm don't actually help developers at all with the "1 only absolutely want one version of this package in my dependency tree" problem (why not in 2018 you might ask?), this solution is probably better than the next best thing which is is instead to keep the peer deps thing going but add an explicit warning/error right now if >1 version of addons is installed. This is what a lot of singleton libraries do.

@Hypnosphi Hypnosphi merged commit 8f8c650 into master Apr 8, 2018
@Hypnosphi Hypnosphi deleted the addons-global-singleton branch April 8, 2018 23:45
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

2 participants