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

React as a regular dependency #26

Closed
ShimiTheFirst opened this issue Jul 13, 2021 · 7 comments
Closed

React as a regular dependency #26

ShimiTheFirst opened this issue Jul 13, 2021 · 7 comments
Labels
bug Something isn't working

Comments

@ShimiTheFirst
Copy link

Hi,

we have been trying this addon in our project and run into an issue with the React hooks warning message. We investigated the cause of it, and we found out we had two different versions of React (and React DOM) installed. One version (17.0.1) came from our package.json a was in the project for a long time. The other one (17.0.2) came with this addon. As I understand it, it’s because it is defined here as a regular dependency (not a peer one) and the very specific version is forced by the local packag.lock.json.

For now, we have fixed it by updating our repo’s version to the 17.0.2 as well, but it had me wonder – if the case was that this package would require 17.0.2 and another package would require eg. 17.0.1 (both listing it as regular dependencies with lock files), is that something I (as a user of these packages) can handle when using both of them at the same time, or would a change in any of those packages be required (eg. listing the dependency as a peer dep instead of a regular one)? 🙂

@SimeonC
Copy link
Owner

SimeonC commented Jul 21, 2021

Hey, thanks for reaching out. Currently the version in the package.json is "^17.0.0" so I didn't think this would cause an issue usually. That said newer versions of NPM have a tendency to do some unexpected thing.

I could possibly remove the dependency altogether which would be for the best, it was just easier coding in react and I didn't have time to dig into rendering non react stories.

@ShimiTheFirst
Copy link
Author

Thanks for the reply. We are using Storybook with React and yarn as a package manager (if that helps with anything) and added this addon as any other dependency. I’m not sure what the problem might be because I develop mostly apps, not packages/libs, but it seems to me that the 17.0.2 version is forced because it’s stated here in the package.lock.json. To not have it locked in there, I think you’d need to move the React from dependencies to peerDependencies (in package.json), but also keep it in devDependencies if you need it installed for your dev purposes. I might not understand it correctly though, as I said I’m not experienced in developing packages/libs 🙂 .

@SimeonC
Copy link
Owner

SimeonC commented Jul 22, 2021

The package-lock you see in this repo is only used for development - you cannot publish it so it can't do what you're saying. https://docs.npmjs.com/cli/v7/configuring-npm/package-lock-json

It's probably due to this open bug in yarn here yarnpkg/yarn#6695 (I use npm)

Currently react is a dependency not a peer dependency as we support usage in non-react storybooks in which case forcing them to install react as a peer would be weird (like a vue project for example)

As far as I can tell this is a yarn issue and not the fault of this package. At some point I'll look into removing the react dependency entirely.

@ShimiTheFirst
Copy link
Author

Thanks for the clarification. I was so focused on our case that I somehow forgot that Storybook is meant to be used with other frameworks as well 😐 . In that case, removing the React dep would make more sense, as you said. Thank you, again, for your time 🙂

@SimeonC
Copy link
Owner

SimeonC commented Jul 22, 2021

Though I did some more research and realised I can't do that anyway - I guess storybook does "something" to handle that anyway. I forgot storybook addon panel UI has to be written in React so there's probably no way around this...

I guess I'll just have to update to match this like you suggested https://github.com/storybookjs/storybook/blob/813c094060857f6c74ed52af891b7d3e87331c68/lib/components/package.json#L73

Has been a couple of months since I last touched storybook addons and today's technically a holiday where I live so sorry for being adamant about not changing anything without checking first.

@ShimiTheFirst
Copy link
Author

For us, it was just this one addon bringing its own version of React so it wasn’t a big deal, we just synced our version of React with this one (17.0.2) but I imagine it would become a bigger problem if we’d use two packages that would each require to use a different version of the same dependency.

Obviously, you can do whatever you want, but I would suggest enjoying the holiday first and coming to this later whenever the time is right for you. At least that is what I would do. Nothing to be sorry about. 🙂

@SimeonC SimeonC added the bug Something isn't working label Jul 30, 2021
SimeonC added a commit that referenced this issue Aug 2, 2021
Issues with react being a dependency instead of a peer-dependency in certain projects/environments.

Fixes #27, #26
@SimeonC SimeonC mentioned this issue Aug 2, 2021
SimeonC added a commit that referenced this issue Aug 2, 2021
Issues with react being a dependency instead of a peer-dependency in certain projects/environments.

Fixes #27, #26
@SimeonC SimeonC closed this as completed Aug 2, 2021
@SimeonC
Copy link
Owner

SimeonC commented Aug 2, 2021

Try the newly released v1.1.4 I moved all my deps to peer and dev so this should be resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants