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

Vite/Svelte: Remove addon-svelte-csf dep #20280

Merged
merged 5 commits into from
Dec 16, 2022
Merged

Conversation

JReinhold
Copy link
Contributor

@JReinhold JReinhold commented Dec 14, 2022

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

@IanVS
Copy link
Member

IanVS commented Dec 14, 2022

Now that the svelte addon supports vite, we should remove the duplicated logic in svelte-vite/src/plugins/csf-plugin.ts

@JReinhold
Copy link
Contributor Author

Now that the svelte addon supports vite, we should remove the duplicated logic in svelte-vite/src/plugins/csf-plugin.ts

Agree. Let's do that in a separate PR, so we can get this peerDeps mismatch out the door.

@IanVS
Copy link
Member

IanVS commented Dec 14, 2022

Will there be any complications from running the same logic twice, I wonder though.

@JReinhold
Copy link
Contributor Author

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",
Copy link
Member

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

Copy link
Member

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

Copy link
Member

@shilman shilman left a 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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"@storybook/addon-svelte-csf": "^3.0.0-next.0",

@IanVS IanVS requested a review from shilman December 16, 2022 12:19
@IanVS
Copy link
Member

IanVS commented Dec 16, 2022

I've pulled in next and removed @storybook/addon-svelte-csf entirely, now that it handles the vite config on its own.

@shilman
Copy link
Member

shilman commented Dec 16, 2022

Thanks for handling this @IanVS ! Looks great!! 💯

@shilman shilman merged commit 4fbb68a into next Dec 16, 2022
@shilman shilman deleted the fix-svelte-csf-peer-deps branch December 16, 2022 12:46
@shilman shilman changed the title Svelte: addon-svelte-csf to ^3.0.0-next.0 Vite/Svelte: Remove addon-svelte-csf dep Dec 16, 2022
@JReinhold
Copy link
Contributor Author

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.
But maybe that doesn't make any sense.

@IanVS
Copy link
Member

IanVS commented Dec 16, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants