[addon-contexts] Should I add global and qs to my package.json?

[sjiep]

Describe the bug
When adding the context addon package to my project as suggested in the README. I receive the following warning from yarn:

warning " > @storybook/[email protected]" has unmet peer dependency "global@*".
warning " > @storybook/[email protected]" has unmet peer dependency "qs@*".

It appears that these two packages are listed as peer dependencies in the package.json of @storybook/[email protected]. However the README doesn't mention that these two package should be installed 'manually'.

Expected behavior
When following the README instructions, not to be left with warnings from yarn.

Either the README should mention we should install these packages together with the addon, or these packages should be added as real dependencies of the addon, not just peer dependencies.

System:

Binaries:
    Node: 10.15.0 - ~/.nvm/versions/node/v10.15.0/bin/node
    Yarn: 1.17.3 - /usr/bin/yarn
    npm: 6.8.0 - ~/.nvm/versions/node/v10.15.0/bin/npm

[leoyli]

@shilman, I actually have the same question on package management here. Should we remove the entire peerDependencies and put qs or global under regular dependencies? I don't see it is really necessary. One thing need to be cared is the dependencies deduplication.

[shilman]

@leoyli I think it's fine to put them as regular dependencies in this case. We should use peer deps when we want to use the user's version. But I don't think it matters in this case?

[leoyli]

@shilman, I think we should use it from @storybook/core, like qs, or global at least (and use ^ to allow dedup). We can use yarn resolution to enforce consistency across the entire workspaces. But now when I install it, I'm always seeing warnings, which seems not too good and confusing.

[shilman]

@leoyli how do you propose we use it from @storybook/core?

typically we just make sure the versions more or less line up across packages in the monorepo and then trust that npm/yarn will do the right thing to dedupe.

in other words adding at as a direct dep of the package shouldn't add any extra overhead unless the version is radically different than @storybook/core's version, which it should never be.

[sjiep]

@shilman @leoyli in my experience adding qs and global as dependencies to the addon-context package should work out nicely (as long as you keep the version range the same/similar). Yarn should nicely hoist/dedupe these dependencies into one.

[sjiep]

I see that now in storybook 5.1.10, react, preact and vue have been adding to peer dependencies. Giving the the following warning from yarn (I'm using React):

warning " > @storybook/[email protected]" has unmet peer dependency "preact@*".
warning " > @storybook/[email protected]" has unmet peer dependency "vue@*".

I'm not entirely sure how to solve that one, you definitely wouldn't want all react, preact and vue as dependencies in storybook itself.

[shilman]

@leoyli why are these peer dependencies at all? i don't see them imported anywhere in addon-contexts. i think this is a bug.

[leoyli]

@shilman.... well I see you the person added it:

https://github.com/storybookjs/storybook/blame/ae8baa22ef8bf019d2cee07a5ef51bb6aa711359/addons/contexts/package.json#L39

It defiantly should not be there, it just meant to make sure the tarball can be built as a optional dependencies. And I can shape a PR to change this altogether as soon as tonight.

[shilman]

@leoyli hahaha well you also see why then 😘

thanks for picking this up 🙇

[shilman]

Gadzooks!! I just released https://github.com/storybookjs/storybook/releases/tag/v5.3.0-alpha.1 containing PR #7675 that references this issue. Upgrade today to try it out!

You can find this prerelease on the @next NPM tag.

Closing this issue. Please re-open if you think there's still more to do.

[sjiep]

@shilman I've updated to v5.3.9 but I see the peer dependencies are still defined in the addons-context package.json. However it looks like the merge from #7675 should have removed them?

[shilman]

@sjiep That's pretty weird. Perhaps some bad conflict resolution at some point? I'm reopening & will try to re-apply these changes to both next and master when I get back to work (on vacation).

[stale]

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

[ahuth]

Hey @shilman , did you ever get a chance to try to re-apply the changes? I can't actually find the commit that adds back the peer deps. Would love to be able to use this addon without needing to install packages I don't actually use (my company unfortunately runs npm ls on CI, and any warnings/errors from that will cause CI failure).

Let me know if you need any help with this, or if a new PR would be helpful.

[shilman]

@ahuth sorry this fell through the cracks. a new PR would be helpful!

Also, FWIW, I plan to deprecate addon-contexts in 6.0 and have created a new addon, addon-toolbars that achieves the same result in a simple/standard/ergonomic/cross-framework way. If you can upgrade, I highly recommend it:

https://github.com/storybookjs/storybook/tree/next/addons/toolbars

[ahuth]

No worries! Also, the toolbars addon looks perfect 👍

[stale]

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

[voxpelli]

Still running into this, what would you want a PR for @shilman? Just the removal of the peer dependencies?

[stale]

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

[shilman]

https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#deprecated-addon-contexts

[stale]

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

[stale]

Hey there, it's me again! I am going close this issue to help our maintainers focus on the current development roadmap instead. If the issue mentioned is still a concern, please open a new ticket and mention this old one. Cheers and thanks for using Storybook!