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

[feat] use preserveValueImports flag in create-svelte #3064

Merged
merged 2 commits into from
Feb 11, 2022

Conversation

dummdidumm
Copy link
Member

TypeScript doesn't know about import usages in the template because it only sees the script of a Svelte file. Therefore preserve all value imports. Requires TS 4.5 or higher, therefore also bump the version of TS and Prettier (the latter in order to deal with the new TS syntax).
Turning on preserveValueImports turns off the advanced but somewhat brittle import transpilation in svelte-preprocess which should give a small performance boost and more robustness.

This requires ESBuild 0.14 or higher. According to Vite's package.json they install a dependency of 0.13.x or higher, so new users should get the version. Pinging @bluwy / @benmccann to ensure I'm not wrong because they know more about Vite than me 😄

Please don't delete this checklist! 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

TypeScript doesn't know about import usages in the template because it only sees the script of a Svelte file. Therefore preserve all value imports. Requires TS 4.5 or higher, therefore also bump the version of TS and Prettier (the latter in order to deal with the new TS syntax).
Turning on preserveValueImports turns off the advanced but somewhat brittle import transpilation in svelte-preprocess which should give a small performance boost and more robustness.
@changeset-bot
Copy link

changeset-bot bot commented Dec 17, 2021

🦋 Changeset detected

Latest commit: 51b362d

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

This PR includes changesets to release 1 package
Name Type
create-svelte 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

@dominikg
Copy link
Member

^0.13 excludes 0.14, so we'll have to wait until vite updates ( should happen with 2.8 )

@dummdidumm
Copy link
Member Author

Oh wow, TIL, caret actually means "do not modify the left-most non-zero element". Thanks for the clarification, marking as draft then in the meantime.

@dummdidumm dummdidumm marked this pull request as draft December 17, 2021 09:48
@Rich-Harris
Copy link
Member

I wonder if we should be using extends to encapsulate things that are important to SvelteKit? This would also allow future versions of Kit to make further changes without requiring existing apps to update their tsconfig.json files.

I haven't used it in the past so I don't know what the caveats are but I imagine it could also be used to keep paths in sync with config.kit.files.lib (though I'm not sure what file you would be extending in that case, since it would be app-specific rather than something inside node_modules/@sveltejs/kit). Maybe we could even tackle #647?

@dummdidumm
Copy link
Member Author

We could very well do this, yes. There's precedence to this already in the Svelte starter template where we extend from @tsconfig/svelte (I repo by Orta for config bases). This tsconfig should live within SvelteKit so we can update it and everyone updating Kit would get it directly.
When we want things like "keep X in sync with Y" then we are talking about another dynamically generated tsconfig though. I'm not sure how to best approach this - would there be a "syncTsconfig" command which looks at things like config.kit.files.lib and generates the results accordingly?

@Rich-Harris
Copy link
Member

I'm imagining something like this...

"extends": "./.svelte-kit/tsconfig.json"

...where .svelte-kit/tsconfig.json is regenerated along with the other generated files, but it has two drawbacks:

  1. it's sort of ugly
  2. it doesn't exist until you first run a svelte-kit command, so a newly-created project would be full of squigglies

I don't know if those are dealbreakers?

@dominikg dominikg mentioned this pull request Feb 9, 2022
5 tasks
@dummdidumm dummdidumm marked this pull request as ready for review February 11, 2022 10:05
@dummdidumm
Copy link
Member Author

Now that TS 4.5 and Vite 2.8 with ESBuild 0.14 are present this is ready for review.

I'd like to keep the suggestions by Rich separate from this PR. I think it might be valuable to have a tsconfig to extend from, but we should think about that once the other type-related stuff settles/is more clear.

@vercel
Copy link

vercel bot commented Feb 11, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

kit-svelte-dev – ./sites/kit.svelte.dev

🔍 Inspect: https://vercel.com/svelte/kit-svelte-dev/AFxVgQfRXAhLFccrkHcyDYbiDLoU
✅ Preview: Failed

@netlify
Copy link

netlify bot commented Feb 11, 2022

❌ Deploy Preview for kit-demo failed.

🔨 Explore the source changes: 51b362d

🔍 Inspect the deploy log: https://app.netlify.com/sites/kit-demo/deploys/62064f77813f810007e95975

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