-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Addon-docs: Vue dependencies cause npm warnings on npm install when not using Vue #8936
Comments
@ndelangen @Hypnosphi @tmeasday any idea what to do about this? don't want to release separate versions of docs per framework... |
@shilman move |
@Aaron-Pool WDYT? ☝️ |
Yeah, that should be fine. Although, of course, it means non-doc users will be installing a dependency they never use 🤷♂️ |
Right now it's the same with |
True, seems like a problem worth solving eventually, perhaps. But probably not a big issue. |
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! |
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! |
Let me get this straight: I don't use vue (I use React), but I want to use Storybook docs addon, so I'm forced to install vue. Is this correct? |
@tx-nestor that is incorrect. It is, in fact, the inverse of that. Vue users that want to use storybook at all are forced to install react (but only as a dev dependency). |
Thanks for the speedy response @Aaron-Pool ! Much appreciated. Unfortunately, that doesn’t seem accurate or align with what we’re seeing in our project. We do not use Vue in our application at all. We use React and Storybook React and have been doing so without issue for a long time, until we installed the storybook docs add on. At that point, we started seeing errors from npm suggesting that vue is required by vue-to-react. The later of which is a dependency of storybook docs. I’m not in front of a computer at the moment, but I will try to create a reference/sample repo I can point you to, so you can see for yourself or at least clarify what I’m doing wrong. Thanks again for you consideration. |
@tx-nestor that's a warning not an error. 😉 we're still working out the package structure, but feel free to ignore that warning for now. |
@tx-nestor Correct! It's throwing that warning because vue is a peer dependency of |
Hello, So, after installing @storybook/addon-docs, I see the following when doing
That's not a warning. That's an error -- npm stating there is an unmet dependency. If I print out the dependency tree, I see this...
So, @storybook/addon-docs has a hard dependency on @egoist/vue-to-react, which has a peer dependency on vue. As such, via the peer dependency, @storybook/addon-docs effectively has a dependency on vue which it does not satisfy on its own and therefore is being pushed up to the end-user of @storybook/addon-docs. Therefore, as a pure React user, if I want to use @storybook/addon-docs, I'm forced to install the core vue framework into my node_modules. Moreover, the vue package does in fact seem to be the core framework. I completely understand if you're still figuring out the intricacies of the package dependencies and I'm not necessarily saying this needs to be fixed ASAP. But, at minimum, I think this issue should be reopened as I think it's a serious problem for users to have to install the entirety of vue when their application and/or UI kit doesn't use it. |
@tx-nestor you don't need to install "the entirety of vue". or any part of vue at all, for that matter. |
@shilman I think there's a misunderstanding here. Indeed, this is not breaking Storybook itself. Storybook still runs without installing vue. So, if you're a pure React user, you don't necessarily need to install vue to get Storybook to work properly. Sorry I didn't make that clear. While that is the case, that's not the issue I'm trying to raise here. This is a matter of breaking the integrity of the npm dependency tree. Generally speaking, if a package introduces a runtime (non-dev) dependency (as @storybook/addon-docs has done here with @egoist/vue-to-react), it is responsible for satisfying all of that dependency's peer dependencies by either installing it as its own runtime dependency or declaring it a peerDependency (in its own package.json). @storybook/addon-docs does neither. We're using hundreds of other packages in our project, none of them break the integrity of the dependency tree except for @storybook/addon-docs. Before installing @storybook/addon-docs, an I suppose whether or not one thinks this is an issue is subjective. But, I feel strongly this breaks the intended use/behavior of |
It's impossible. Peer dependencies can only be satisfied on end app level |
@Hypnosphi It’s not impossible. Peer dependencies can be satisfied at any point in the hierarchy in which they are introduced. The issue is @storybook/addon-docs neither satisfies the vue dependency itself nor does it specify it as a peerDependency. It just ignores it and let’s the end user deal with it (“end app level”, as you called it). Which is annoying because my project doesn’t use vue at all — yet I would have to install it if I want to maintain a fully satisfied dependency graph/tree. Perhaps people just don’t care about this as much as we do at our company — and that’s fine. I’m willing to concede this isn’t a problem with Storybook itself. But it is a problem with how they’re introducing dependencies in this particular add-on. |
Here's why it's not true: |
@tx-nestor let's shift the conversation a bit from who's right or wrong here. What would you propose we do to fix the problem? The only solution I see is to divide the docs add-on into a ton of framework specific packages (like the core storybook app), but that's going to introduce a lot of overhead and headache to the maintenance burden for the add-on, and I don't think the team is willing to do that just to preserve the purity of the npm dependency tree. Is that the solution you had in mind? Or did you have a simpler alternative to propose? |
@Aaron-Pool Yeah, sorry, I wasn’t trying to prove I’m right, but rather express my concern for this issue (which was closed). The more I read into it (thanks for links @Hypnosphi), the more I’m uncovering that npm’s dependency graph and resolution is complex and imperfect (particularly re: their peer and optional dependency feature). Re: how to fix...
Yes, that may be the only way to resolve this given the way npm dependencies work at the moment. FWIW, I think the Storybook team has done a great job with figuring out how to support multiple frameworks — honestly, might be one of the best case studies on this sort of thing I’ve seen. Would you need to create a separate docs add on for every framework upfront? Or, could you just create one for any framework that breaks the mold? Also, I see Storybook is using Lerna — Does that not eliminating the overheard of introducing new packages under the @storybook scope? Humor me for a minute, for my wild idea of the day... Could you potentially solve this problem with dynamic imports against a Storybook CDN? Or, maybe even just unpkg or cdnjs. As opposed to relying on static bundling. |
We could also make @shilman WDYT? |
Also, this seems like a viable option, not requiring a breaking change: #8936 (comment) |
Yeah, I like moving it to the storybook/Vue package over making it a peer dependency. I think peer dependencies are confusing for most end users and should only be used in cases that really necessitate it. |
If we move
So I think we actually need both "optional peer deps" (to deal with point 2) AND to move |
@shilman |
Thanks @shilman - confirming I'm happy with the approach above after reading this thread. Fully agree in it's current form it's at best confusing and at worse poor practise. Thanks for all thea activity and willingness to fix! |
@tomdev10 Would you be able to contribute the fix perhaps? |
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! |
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! |
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! |
Perhaps this issue can be resolved by using See npm/cli#224 |
@dfernandez-asapp That's interesting. I have not used it before but worth a shot. cc: @shilman @Hypnosphi Any idea if this would work and resolve this problem? |
@tx-nestor yes, agree it looks promising. my current thinking is to put all framework-specific dependencies in the framework package (e.g. |
@shilman No worries re: v6 blockers. I've pretty swamped myself. BUT, I think I can spare some time do some experimenting locally by hacking with node_modules and seeing how npm behaves. I can also publish a package to our internal private package repository and see if the peerDependenciesMeta works as documented. If my experiments look promising, I'll open an official PR in this repo. |
FWIW we're using this successfully in Storybook already, but I didn't connect the dots until @dfernandez-asapp 's comment: |
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! |
Son of a gun!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.0.0-rc.22 containing PR #11759 that references this issue. Upgrade today to try it out! You can find this prerelease on the Closing this issue. Please re-open if you think there's still more to do. |
@shamin Confirmed fixed when I install the v6 release candidate. Thanks SO MUCH! Can't wait to see what else is in store for Storybook v6! |
FYI this has been fixed properly in https://github.com/storybookjs/storybook/releases/tag/v6.5.0-alpha.48 To test it out:
|
Describe the bug
To Reproduce
Setup Storybook with addon-docs with a different framework (e.g. React)
Expected behavior
Maybe there needs to be separate packages for Vue / each framework support?
System:
Please paste the results of
npx -p @storybook/cli@next sb info
here.The text was updated successfully, but these errors were encountered: