search

SimeonC / storybook-xstate-addon

A storybook addon to assist with writing stories that rely on xstate
https://SimeonC.github.io/storybook-xstate-addon/
MIT License
54 stars 6 forks source link

React as a regular dependency #26

Closed ShimiTheFirst closed 2 years ago

ShimiTheFirst commented 3 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 https://github.com/yarnpkg/yarn/issues/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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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