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

Chore/update vite plugin svelte #2343

Merged
merged 8 commits into from
Sep 1, 2021
Merged

Conversation

dominikg
Copy link
Member

@dominikg dominikg commented Sep 1, 2021

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

with kit already updated to vite-2.5.3 , users can update to vite-plugin-svelte@1.0.0-next.20 on their own but this should increase adoption-rate and visibility for a feature that should solve a lot of pain people were having with transitive cjs dependencies.

Not sure if patch or minor 🤷‍♂️

@changeset-bot
Copy link

changeset-bot bot commented Sep 1, 2021

🦋 Changeset detected

Latest commit: f220efc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@bluwy
Copy link
Member

bluwy commented Sep 1, 2021

Should we remove the old auto-exclude code here too? (even though it should still work without). And we could also consider removing this faq section.

@dominikg
Copy link
Member Author

dominikg commented Sep 1, 2021

yes, i think we should, good catch @bluwy . No point in sveltekit and vite-plugin-svelte doing the same thing.
Also updating the docs to mention the new automatic behavior is a good idea. Should be done on vite-plugin-svelte tho with a link from here.

@benmccann
Copy link
Member

Agreed on removing the duplicate functionality from SvelteKit

I'd maybe just remove one sentence of the FAQ:

However, in the meantime, you can workaround this issue by adding the dependency to vite.optimizeDeps.include in svelte.config.js.

That should be done automatically now. The rest still seems relevant

@dominikg
Copy link
Member Author

dominikg commented Sep 1, 2021

docs change in vite-plugin-svelte sveltejs/vite-plugin-svelte#158

@bluwy
Copy link
Member

bluwy commented Sep 1, 2021

I'd maybe just remove one sentence of the FAQ:

@benmccann I think the first and second paragraph of that section can be removed though, as it should never happen anymore. The second paragraph also has a broken link. Removing those would also made the third paragraph redundant.

Perhaps we could replace with the ESM-only dep issue, or some other issues listed at #2086? as they would now be more common.

@benmccann
Copy link
Member

The link in the second paragraph of the FAQ appears to be broken, so yeah we can remove the second paragraph. Perhaps we should add something at the very top of the SvelteKit FAQ that points at https://svelte.dev/faq and https://github.com/sveltejs/vite-plugin-svelte/blob/main/docs/faq.md

For the first perhaps we can rewrite it to something like:

SSR in Vite is not yet stable. Libraries work best with Vite when they distribute both CJS and ESM in their package and you may wish to work with library authors to make this happen.

@benmccann benmccann force-pushed the chore/update-vite-plugin-svelte branch from 1b91679 to 8059185 Compare September 1, 2021 16:03
@benmccann
Copy link
Member

benmccann commented Sep 1, 2021

Oh crap. I thought I was being helpful and making some docs changes to this PR, but accidentally pushed them to master instead. @dominikg I just dropped your last commit from this PR to avoid the merge conflict and added a new commit to fix the lint error by formatting. I can merge this then if everything looks okay to you guys now

@benmccann benmccann force-pushed the chore/update-vite-plugin-svelte branch from dcfb23b to 9f9cf67 Compare September 1, 2021 16:06
@dominikg
Copy link
Member Author

dominikg commented Sep 1, 2021

maybe wait a little, @bluwy found an edge case where having packages without a valid export like @types/node as dependency of a svelte dep (eg routify) breaks vites optimizer. We're adding an option to exclude nefarious packages or honor existing excludes.

So there will be next.21 soonish

@dominikg
Copy link
Member Author

dominikg commented Sep 1, 2021

next.21 is released

@benmccann benmccann merged commit 0cb830d into master Sep 1, 2021
@benmccann benmccann deleted the chore/update-vite-plugin-svelte branch September 1, 2021 21:13
@github-actions github-actions bot mentioned this pull request Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants