-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Vite/Svelte: Remove addon-svelte-csf dep #20280
Conversation
Now that the svelte addon supports vite, we should remove the duplicated logic in |
Agree. Let's do that in a separate PR, so we can get this peerDeps mismatch out the door. |
Will there be any complications from running the same logic twice, I wonder though. |
This is probably what's causing storybookjs/addon-svelte-csf#83 actually. |
@@ -51,7 +51,7 @@ | |||
"prep": "../../../scripts/prepare/bundle.ts" | |||
}, | |||
"dependencies": { | |||
"@storybook/addon-svelte-csf": "^2.0.0", | |||
"@storybook/addon-svelte-csf": "^3.0.0-next.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this both a dependency and a peer dependency? I think it shouldn't be a dependency @JReinhold @IanVS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact it should be removed entirely, from both peer and dependencies. We included it previously in peer dependencies because we were using it in the csf-plugin
. But now we don't need to with the latest version of the addon.
@JReinhold there are also some type definition files that can be removed now too, along with plugins/csf-plugin.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LGTM
@@ -51,7 +51,7 @@ | |||
"prep": "../../../scripts/prepare/bundle.ts" | |||
}, | |||
"dependencies": { | |||
"@storybook/addon-svelte-csf": "^2.0.0", | |||
"@storybook/addon-svelte-csf": "^3.0.0-next.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"@storybook/addon-svelte-csf": "^3.0.0-next.0", |
# Conflicts: # code/frameworks/svelte-vite/package.json # code/yarn.lock
I've pulled in next and removed |
Thanks for handling this @IanVS ! Looks great!! 💯 |
addon-svelte-csf
to ^3.0.0-next.0
Thanks for finishing this up @IanVS ! I was way too hasty in this PR. I also initially thought about removing addon-csf entirely from does, but I convinced myself that it should stay as peer deps and optional, because that would allow us to specify which version was required, so users didn't end up installing the old version. |
We don't use peer deps for any other community addons, so I didn't think it made sense for this one either. It's up to the user to install the correct version of whatever plugin they're using for their version of Storybook. If anything, we could update some documentation to make it clear what has to be installed. That's just my humble opinion, though. Happy to be overruled if others disagree. |
Upgrade peer dependency
@storybook/addon-svelte-csf
to^3.0.0-next.0
.Same PR on the other end: storybookjs/addon-svelte-csf#81. Together they (hopefully) fix storybookjs/addon-svelte-csf#82
Also fixes storybookjs/addon-svelte-csf#83