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: native TypeScript support #9482

Merged
merged 22 commits into from
Nov 20, 2023
Merged

feat: native TypeScript support #9482

merged 22 commits into from
Nov 20, 2023

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Nov 16, 2023

Thought I'd have a quick look to see how feasible it would be to support TypeScript natively via acorn-typescript. Props to @gtm-nayan for the initial prototype, which this leans on heavily.

Turns out it's quite reasonable, at least to the extent that I've tested it. Notes:

  • Ironically, this makes typechecking in the parser itself much harder — I had to slap a bunch of ts-expect-error annotations on it to get rid of the red squigglies
  • acorn-typescript adds some gubbins to nodes (loc.start.index and loc.end.index) that we need to delete. It also seems to forget to set end on a node when it has a type annotation. Not sure why. Both workaroundable
  • We thought each blocks might be tricky, because as in {#each things as thing} looks like a type assertion, and thus part of the first expression. Turns out we can solve that in most cases quite simply, just by unwrapping the assertion and rewinding the parser to before the as
  • A trickier case is {#each x as { y = z }}, because x as { y = x } won't parse. Most Pattern nodes can happily masquerade as Expression nodes, but AssignmentPattern nodes cannot. In these cases we employ a dreadful hack: we look for the most recent occurrence of as, blank out everything from there onwards, and try parsing the expression again. We keep doing that until we get a valid expression. This is absolutely horrific but it seems to work
  • Another awkward case is snippets (which are the very reason we need this so badly in the first place). Since we can't use parseExpressionAt to read the snippet parameter (because of the Pattern thing, again), we have to read the parameter using our existing pile of hacks, then construct some artificial code to coax Acorn into giving us the type annotation, if such there be
  • We can now use TypeScript inside things like inline event handlers.
  • I'm not stripping the types out. They're harmless (though we need to unwrap TSAsExpression and TSNonNullExpression nodes at transform time), and could potentially provide some future value
  • Right now, it parses everything in TypeScript mode. It doesn't (for example) only do it for components that have a <script lang="ts">. Truthfully, I'm not sure what we should do here. lang="ts" is a convenient place to put the setting, though it's also slightly weird if it also controls how the template is parsed. (What if <script context="module"> disagrees with <script>?)
  • If we did always allow TypeScript, or at least supported it natively behind some option like lang="ts", then would we no longer need the preprocessor?
  • How does this interact with language-tools?
  • I haven't investigated whether this change has any performance implications for the parser

Svelte 5 rewrite

Please note that the Svelte codebase is currently being rewritten for Svelte 5. Changes should target Svelte 5, which lives on the default branch (main).

If your PR concerns Svelte 4 (including updates to svelte.dev.docs), please ensure the base branch is svelte-4 and not main.

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
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests and linting

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

Copy link

changeset-bot bot commented Nov 16, 2023

🦋 Changeset detected

Latest commit: 46d4073

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

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

Copy link

vercel bot commented Nov 16, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
svelte-5-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 20, 2023 4:11pm

@Rich-Harris
Copy link
Member Author

No idea why there's a lint issue, can't reproduce locally

@brunnerh
Copy link
Member

  • Right now, it parses everything in TypeScript mode. It doesn't (for example) only do it for components that have a <script lang="ts">. Truthfully, I'm not sure what we should do here. lang="ts" is a convenient place to put the setting, though it's also slightly weird if it also controls how the template is parsed. (What if <script context="module"> disagrees with <script>?)

If it's a compiler thing with this change, would it make sense to have it as a compiler option so lang="ts" is no longer needed/used (but maybe can be kept as an annotation)?

People that use TS could then just turn that on in the config to have TS everywhere and in special cases where it should be off, <svelte:options> could be used.

@gtm-nayan
Copy link
Contributor

No idea why there's a lint issue, can't reproduce locally

Seems like the formatter plugin is linking to the in-branch version of Svelte, and ends up printing as twice in each loops

image

@dummdidumm
Copy link
Member

dummdidumm commented Nov 16, 2023

Some thoughts:

  • the SnippetBlock and RenderTag should both probably just have one node, an expression. This simplifies parsing, because you don't need to the whole back-and-forth, and we can analyze that the expression is valid in the validation phase. Additional benefits:
    • opens up the door for more flexibility in the future: we could allow ternary expressions in the render tag, as long as it ends in a call expression
    • parser throws errors less often, which is benefitial for language tools. Right now, you're getting a parser error until you have written out an identifier plus the braces, which makes autocompletion worse
  • lang="ts" implications: right now it's not allowed from a language tools standpoint to mix and match, i.e. if one is lang="ts", both are. For the preprocessor they are not, which we could change. Beyond that, I think we need to keep the tag. We had a default language language setting for a little while in VS Code / svelte-preprocess but removed it because it's too brittle:
    • syntax highlighting is slightly different between JS and TS, and there's no way to tell VS Code to choose one or the other grammar on the fly, it's a static thing at extension startup
    • TypeScript is all synchronous, svelte.config.js can be ESM and therefore needs to be loaded asynchronously in order to read a potential "treat these as TS" flag. That doesn't play nice together, as the info whether or not something should be treated as TS by default might come in "too late". Prettier and eslint may have similar restrictions
    • related, we can't "guess" what language this is in, and if someone wanted to use JS with JSDoc, and we treat everything as TS by default, those JSDoc tags would be ignored, causing errors
  • while we're at it, there's a (currently experimental) attribute named generics which you set on the instance script tag to put generics in them. It would be good to either not treat braces as expression tags on script/style tags or to at least special-case the generics attribute. If we don't, people can't write things like generics="T extends { foo: boolean }"
  • with this change, if we don't strip types ourselves, then we probably need to adjust the toolchain to run a TS->JS transformation after the Svelte compiler, since it may contain type annotations. cc @dominikg / @bluwy

@tomblachut
Copy link

tomblachut commented Nov 16, 2023

Hi! Hopefully I understood correctly, as in each will never be parsed as a type assertion, right? What about parentheses

{#each (cats as any[]) as { id, name }, i}

should this be disallowed or supported/tested?

@ctjlewis
Copy link

This is great! We could even add TypeScript to this whole repository - what does everyone think?

@Rich-Harris
Copy link
Member Author

as in each will never be parsed as a type assertion

It will! We just need to do some hoop-jumping to differentiate between the different types of as. But it works, with or without parens: demo

@xamir82
Copy link
Contributor

xamir82 commented Nov 16, 2023

An application-wide lang: 'ts' setting would be nice actually, though apparently it'd be problematic if it's put in svelte.config.js as dummdidumm says.

@Rich-Harris
Copy link
Member Author

Per discussion elsewhere, we now only parse in TypeScript mode if there's a <script lang="ts"> in the component. This guards against future breakage, while leaving us the flexibility to add a default via a compiler option in future.

@@ -10,6 +10,7 @@
"noErrorTruncation": true,
"allowSyntheticDefaultImports": true,
"verbatimModuleSyntax": true,
"skipLibCheck": true,
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this? We should avoid setting this, as it's going to hide any type errors we introduce inside d.ts files.

Copy link
Member Author

Choose a reason for hiding this comment

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

because of type checking errors that were happening inside acorn-typescript. i'm not sure if there's a way to disable checks for a specific library? if not then to remove this we would need to fork acorn-typescript or work with the maintainers (hi @ota-meshi!) to figure out those errors. That would be preferable of course, I just didn't include that work here because it was initially more of a PoC than anything

@dominikg
Copy link
Member

Per discussion elsewhere, we now only parse in TypeScript mode if there's a <script lang="ts"> in the component. This guards against future breakage, while leaving us the flexibility to add a default via a compiler option in future.

If we were to remove it, vite import scanner would need a new way to determine if a .svelte file script has typescript in it too. Right now it does look for lang="ts" for a few sfc extensions on it's own (.vue and .svelte at least)

@dominikg
Copy link
Member

  • with this change, if we don't strip types ourselves, then we probably need to adjust the toolchain to run a TS->JS transformation after the Svelte compiler, since it may contain type annotations. cc @dominikg / @bluwy

hmm, i think in that case we'll have to somehow tell vite that we emit ts and not js (does that work with a faux .ts extension in resolve?). It could work but what about other bundlers that dont have vites pre/post ordering? not sure it would work with rps and no clue at all about svelte-loader or others.

What would be the reason to not emit plain js?

@Rich-Harris
Copy link
Member Author

If we were to remove it, vite import scanner would need a new way to determine if a .svelte file script has typescript in it too

There are two possibilities — reading from svelte.config.js...

export default {
  compilerOptions: {
    typescript: true
  }
};

...or reading from tsconfig.json:

{
  "svelte": {
    "typescript": true
  }
}

The first feels nicer, but creates problems because the TypeScript plugin also needs this information, and it has to work synchronously. That's easy with tsconfig but involves piles of hacks otherwise.

What would be the reason to not emit plain js?

We do emit plain JS

@dominikg
Copy link
Member

dominikg commented Nov 19, 2023

That's easy with tsconfig but involves piles of hacks otherwise.

Except that now you need a pile of hacks to read it from tsconfig. I'd recommend tsconfck for this but it's async. Native ts utilities are orders of magnitude slower.

I also don't think that vite would implement a different scheme than lang="ts" for sfc import scanning in core, so we'd need to contribute something to vite that allows us to tell it. For the short-term i don't think we can remove it in favor of a project wide config. There had been a previous attempt at this by svelte-preprocess but it was removed.

In the long term, i believe a project config for this should be in svelte.config.js, not only because it's nicer but because it is svelte config, not typescript config.
If sync reading is an issue, then we can find a way to address this by making it sync readable without execution (mandate it being an fs read parsable string for example export const typescript = true)

A more generic way of doing this would be to use frontmatter (in svelte.config.js AND .svelte files) to have sync parsable compiler options on a global/file basis. This could replace svelte:options aswell.

last but not least if you really want to put it into a json file for sync parsing, i'd prefer using package.json over tsconfig.json. This is a lot easier to find and parse.

We do emit plain JS

great, so just to confirm, this addresses the point from @dummdidumm above confirming that the compiler strips types or is there another js->js transform needed to remove something? If yes, I think it should be done by the compiler to avoid it having to be implemented in different places.

@dummdidumm
Copy link
Member

Before going ahead I'd like to make sure this works with language tools and the prettier plugin

@birkskyum
Copy link

just curious, but how come these acorn plugins are still working with rollup 4? seemed like rollup 4 all about sunsetting acorn

@gtm-nayan
Copy link
Contributor

We don't use acorn through rollup, we import it in the compiler and our files go through the transform hook rather than the acorn plugin interface so any changes to rollup in that regard wouldn't affect it.

@Spenhouet
Copy link

Since Svelte loves putting logic in file names, here another suggestion: Naming files *.js.svelte or *.ts.svelte to specify the language.

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.

10 participants