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: don't add svelte-loader nor svelte when initializing Svelte projects #18799

Merged
merged 5 commits into from
Jan 13, 2023

Conversation

benmccann
Copy link
Contributor

What I did

svelte-loader should not be added indiscriminately. It is only needed for webpack projects, but Svelte projects use Vite by default, so it's mostly wrong to add it. Users will have set this up already if they're using Svelte and webpack

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

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

@benmccann benmccann mentioned this pull request Jul 27, 2022
3 tasks
@IanVS IanVS added patch:yes Bugfix & documentation PR that need to be picked to main branch svelte labels Jul 27, 2022
@benmccann benmccann changed the title Update index.ts bug: svelte-loader should not be added to all Svelte projects Jul 27, 2022
@IanVS IanVS added cli and removed svelte labels Jul 27, 2022
@MichaelArestad
Copy link
Contributor

@j3rem1e can you please review this when you get a chance?

@ndelangen ndelangen changed the base branch from main to next January 13, 2023 10:54
@ndelangen ndelangen added the bug label Jan 13, 2023
@JReinhold
Copy link
Contributor

Isn't this superseded by #19263 by @IanVS, that only adds the svelte-loader in Webpack based projects?

Or do we not want to add the loader at all, because we assume webpack setups already have that? If they already have that then this won't make a difference though, right?
If we never want to add it - not even for webpack projects - wouldn't the same hold true for the svelte package, don't we expect that to be present too always?

I haven't tried any of this out, but my hunch tells me that we shouldn't add any extra packages at all, because we can assume that they're already there, since they are actually essential packages and not extra packages.

@benmccann
Copy link
Contributor Author

I would assume both svelte-loader and Svelte are already present and agree we shouldn't need to add either of them. I've updated this PR accordingly

@JReinhold JReinhold changed the title bug: svelte-loader should not be added to all Svelte projects Svelte: don't add svelte-loader nor svelte when initializing Svelte projects Jan 13, 2023
@JReinhold JReinhold added maintenance User-facing maintenance tasks and removed bug patch:yes Bugfix & documentation PR that need to be picked to main branch labels Jan 13, 2023
@JReinhold JReinhold merged commit 67e2899 into next Jan 13, 2023
@JReinhold JReinhold deleted the benmccann-patch-1 branch January 13, 2023 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants