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

Svelte: Make svelte-loader optional dependency #18645

Merged
merged 5 commits into from Jul 9, 2022

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented Jul 6, 2022

What I did

Make svelte-loader optional so that we don't add webpack support when using @storybook/builder-vite. You just need one or the other

How to test

  • Is this testable with Jest or Chromatic screenshots? - probably doesn't need this type of test
  • Does this need a new example in the kitchen sink apps? - no. existing apps work the same, but just install more quickly
  • Does this need an update to the documentation? - no. existing apps work the same, but just install more quickly

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented Jul 6, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 807c5be. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


Successfully ran 1 target

Sent with 💌 from NxCloud.

@ndelangen
Copy link
Member

ndelangen commented Jul 6, 2022

Hello @benmccann Thank you for this contribution, unfortunately We're working on a massive refactor that splits apps into frameworks, presets and renderers. Frameworks will introduce a builder.
You can find this work in the future/base branch.

I don't think I can merge this PR, because it adds dependencies to the svelte app in such a way users would always / often get both builders. The refactor I described above, is all about preventing that.

I'm appreciative of your time, energy and work you put into this PR. I'm sorry for closing it.

The future/base should make it's way into next soon. @shilman is on vacation right now, and so we'll wait for a bit. Also future/base needs to mature just a little bit more.

@ndelangen ndelangen closed this Jul 6, 2022
@benmccann
Copy link
Contributor Author

benmccann commented Jul 6, 2022

I don't think I can merge this PR, because it adds dependencies to the svelte app in such a way users would always / often get both builders.

I don't think that's true because @storybook/builder-vite was added as optional. I'm happy to remove any reference to @storybook/builder-vite though as it's in no way necessary. While we're waiting for the larger refactor, it would be extremely helpful if we could still consider the portion of this PR which makes svelte-loader optional. I discussed this with @IanVS before sending the PR. Is it possible we could reopen this?

@IanVS
Copy link
Member

IanVS commented Jul 6, 2022

Peer deps are weird, some package managers install them automatically, some don't. I'd suggest removing the vite builder from this PR, and just keeping the optional dep on the webpack loader. I think that still makes sense to be optional, since this addon can be used with the vite builder without it.

@ndelangen ndelangen reopened this Jul 6, 2022
@ndelangen
Copy link
Member

ndelangen commented Jul 6, 2022

Absolutely!

@benmccann
Copy link
Contributor Author

benmccann commented Jul 6, 2022

Thanks guys! I've made that change

IanVS
IanVS approved these changes Jul 7, 2022
Copy link
Member

@IanVS IanVS left a comment

LGTM, thanks.

Except, it looks like you'll need to run yarn install, and check in the lockfile, to get the Danger JS ci job passing.

@benmccann
Copy link
Contributor Author

benmccann commented Jul 7, 2022

oops. updated! thanks for taking a look 😄

@IanVS
Copy link
Member

IanVS commented Jul 7, 2022

Hmmmm, I'm not sure why it's failing now. It seems it can't get some user information, but I'm not familiar with this CI check or how it works.

@IanVS
Copy link
Member

IanVS commented Jul 8, 2022

Does anyone know what might be happening with the CI on this PR?

@shilman
Copy link
Member

shilman commented Jul 8, 2022

@IanVS I've just added @benmccann to the Storybook github org. Once he accepts his invite the CI should run. It's a security thing.

@shilman
Copy link
Member

shilman commented Jul 8, 2022

@IanVS @benmccann does this need to be patched back to 6.5?

@IanVS
Copy link
Member

IanVS commented Jul 8, 2022

It would be nice if it were, yeah.

@shilman shilman changed the title cleanup: make svelte-loader optional Svelte: Make svelte-loader optional dep Jul 8, 2022
@shilman shilman added the patch Bugfix & documentation PR that need to be picked to master branch label Jul 8, 2022
@benmccann
Copy link
Contributor Author

benmccann commented Jul 8, 2022

I accepted the invite. I'm not quite sure how to kick off the CI again. I tried rerunning the jobs, but nothing appears to be happening

Let me know if there's anything I need to do for the 6.5 backport. Otherwise I'll assume you guys are handling that

@shilman
Copy link
Member

shilman commented Jul 8, 2022

@benmccann try merging in next to create a new commit. i'll handle the patch at some point soon, don't worry about it 👍

@benmccann
Copy link
Contributor Author

benmccann commented Jul 8, 2022

I merged in next, but it doesn't seem to be running the CI yet. Though I think it's unlikely this change will affect anything the CI tests anyway, so we could probably merge it without running the CI

@shilman shilman changed the title Svelte: Make svelte-loader optional dep Svelte: Make svelte-loader optional dependency Jul 9, 2022
@shilman shilman merged commit a89c98d into storybookjs:next Jul 9, 2022
31 of 38 checks passed
@benmccann benmccann deleted the patch-3 branch Jul 21, 2022
@shilman shilman added the picked Patch/release PRs cherry-picked to master/release branch label Jul 26, 2022
shilman added a commit that referenced this issue Jul 26, 2022
Svelte: Make `svelte-loader` optional dependency
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app: svelte maintenance patch Bugfix & documentation PR that need to be picked to master branch picked Patch/release PRs cherry-picked to master/release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants