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

add typescript support #13

Merged
merged 33 commits into from
Dec 17, 2024
Merged

Conversation

manuel3108
Copy link
Contributor

@manuel3108 manuel3108 commented Aug 11, 2024

Closes #10

Goal:
Provide a tool that can properly print svelte-ast including their appropriate JS / TS

Relates xeho91/svelte-ast-print#86
Relates svelte-add/svelte-add#193 / svelte-add/svelte-add#507
Previous attempt, not invoking esrap: xeho91/svelte-ast-print#90 (tried using recast, failed)

State of this PR
This is a POC. My main goal was to make the failing tests of svelte-ast-print pass. I wrote a few tests for some basic functionality, but there are 100% a lot more things we might need to think about.

Notes

  • Includes code from chore: use vitest #12 as test do not pass otherwise for me. If those changes are not wanted, I can easily remove them.
  • We might need to consider using ast-types instead of @types/estree, as ast-types provides correct types for type annotations. @types/estree explains how they could work in the documentation, but do not actually the provides for that. All TS related properties are untyped, thus the many // @ts-expect-error comments Done by using @typescript-eslint/types
  • Uses acorn-typescript under the hood, same as svelte

Todos

  • check typing (see note about ast-types above)
  • enable sandbox to run typescript codes
  • way more tests

@xeho91
Copy link

xeho91 commented Aug 12, 2024

as-types is Esprima specification compatible.
Meanwhile both acorn and typescript-acorn uses EStree specification. And also Svelte with parse/compile functions.

I don't have experience with Esprima, at first glance on their repository, they don't seem to overlap with ESTree. Are you sure this is the right direction to solve typing issues?

@manuel3108
Copy link
Contributor Author

as-types is Esprima specification compatible.
Meanwhile both acorn and typescript-acorn uses EStree specification. And also Svelte with parse/compile functions.

I don't have experience with Esprima, at first glance on their repository, they don't seem to overlap with ESTree. Are you sure this is the right direction to solve typing issues?

No I'm not - Will need to check. This was my first guess, as we are using ast-types in svelte-add, but indeed we are currently not using acorn there.

@Rich-Harris
Copy link
Owner

I'm also not sure that the ast-types interfaces match what acorn-typescript uses, since it doesn't use ast-types. I wonder if @typescript-eslint/typescript-estree is the answer?

Either way, this is super cool, thank you — would love to see this land!

@dummdidumm
Copy link
Collaborator

If you mean "replace Acorn in Svelte with TS-Estree" - it's probably going to be slower: typescript-eslint/typescript-eslint#1003

@Rich-Harris
Copy link
Owner

I fiddled around with it a bit last night and concluded it probably wouldn't work for us because there's no equivalent of parseExpressionAt

@manuel3108
Copy link
Contributor Author

Regarding the ast-types thing, this @typescript-eslint/types could be the solution. Seems to be providing correct intellisense. Will give it a shot

/** @param {import('@typescript-eslint/types').TSESTree.FunctionDeclaration} test */

Copy link
Contributor Author

@manuel3108 manuel3108 left a comment

Choose a reason for hiding this comment

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

This is way better! But sadly, not good. eslint and @typescript-eslint/types handle comments in a special way (globally) and therefore we are currently left with 9 type errors regarding comments.

But I think we should be able to find a workaround here.

Edit: And the autocompletion is painfully slow. Will check where this is coming from
Edit 2: Seems to be related to our own types.

type NodeOf<T extends string, X> = X extends { type: T } ? X : never;

type Handler<T> = (node: T, state: State) => undefined;

export type Handlers = {
	[K in AST_NODE_TYPES]: Handler<NodeOf<K, TSESTree.Node>>;
};

src/handlers.js Outdated Show resolved Hide resolved
src/handlers.js Show resolved Hide resolved
@manuel3108
Copy link
Contributor Author

manuel3108 commented Aug 24, 2024

Are you aware of any "full typescript feature list" so that we can see which features we are still missing? I'm currently trying to work things out with the typescript docs, but that seems pretty hard. If not, do you maybe have typescript file in mind, that we can use to determine if we are printing the same output?

From the top of my head, here is what we already have / what's missing:

  • decorators
  • generics and generic types
  • typed object patterns
  • typed functions (params and return value)
  • basic types for variables etc (number, string, bool, any)
  • typed imports (import type A from 'foo' and it's derivates
  • utility types (need to check / should work)
  • enums
  • as
  • satisfies
  • class property modifiers (private, etc)
  • type combinations (tuple, union, intersection, conditional)
  • interfaces

I don't think this PR should necessarily implement "all" of typescript. We should probably cover the most common use-cases and add the rest later on a as-needed basis.

@xeho91
Copy link

xeho91 commented Sep 2, 2024

Hi, I am still a noob in parsers and stuff. But my question is why not use oxc rather than acorn. I think the bundle size for oxc-parser is smaller than acorn-typescript. And also it might be faster than acorn-typescipt, since oxc is written in rust (sorry no proof, just guessing).

Here are bundle size
acorn-typescript
image

oxc-parser
image

  1. This package isn't about parsing, but rather about doing it in reverse (hence the name of this package). To put it another way: return the parsed AST object back to string.
  2. Attempt to use acorn-typescript in this PR was to extract/use the interfaces for parsed AST nodes related to TypeScript. There is no official extension of existing @types/estree which doesn't include TypeScript.
  3. Bundle size doesn't include downloaded binary (compiled with Rust) which comes with installing/adding this package to your project - oxc-parser includes a script to determine which operating system you're using.

@manuel3108
Copy link
Contributor Author

manuel3108 commented Sep 3, 2024

I gave oxc-parser a go, but it does not seem to be there yet. As described above, the parser was never the problem, but the types that are generated from the parser.

Edit: Additionaly they seem to mix esprima and estree specs together, which makes it painfull to work with types afterwards.


Functionality wise this PR is ready in my opinion (expect the two open conversations above). Typing wise, there are still major problems as described above. To put things into numbers: On main we currently have 4 comments with @ts-expect-error, 20 on this branch. Additionally intellisense is painfully slow, due to the new typing library.

That's why im not undrafting this PR at this point

@manuel3108
Copy link
Contributor Author

Undrafted, as this is completed functionality wise.
The only remaining potential issue is in this review comment: #13 (comment)

To sumarize a lot of the previous comments:

Typing wise, there are still major problems as described above. To put things into numbers: On main we currently have 4 comments with @ts-expect-error, 20 on this branch. Additionally intellisense is painfully slow, due to the new typing library.

If we can fix the review comment above and we can agree on the following thing, I think we are good to go.

I don't think this PR should necessarily implement "all" of typescript. We should probably cover the most common use-cases and add the rest later on a as-needed basis

@manuel3108 manuel3108 marked this pull request as ready for review September 14, 2024 14:49
package.json Outdated Show resolved Hide resolved
@manuel3108 manuel3108 changed the title add typescript support (poc) add typescript support Sep 20, 2024
Copy link

@xeho91 xeho91 left a comment

Choose a reason for hiding this comment

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

Found some small chores while studying your changes

src/handlers.js Outdated Show resolved Hide resolved
src/handlers.js Outdated Show resolved Hide resolved
src/handlers.js Outdated Show resolved Hide resolved
src/handlers.js Outdated Show resolved Hide resolved
@xeho91
Copy link

xeho91 commented Dec 16, 2024

GitHub discussions don't create mentions, so I'll create one.

This case would help improve the current state of the svelte-docgen package (which is also an ongoing work in progress). I've described how there: svelte-docgen/svelte-docgen#17 (comment)

@Rich-Harris Rich-Harris merged commit 4e6ef0c into Rich-Harris:main Dec 17, 2024
6 checks passed
@Rich-Harris
Copy link
Owner

this is incredible — thank you everyone, and apologies for the delay in merging

@korywka
Copy link

korywka commented Dec 17, 2024

Guys, the internet is broken now 🥹

@frederichoule
Copy link

More context on @korywka 's comment:

sveltejs/svelte#14741
sveltejs/vite-plugin-svelte#1053

@AlbertMarashi
Copy link

sveltejs/svelte#14743

Also getting issues due to this

@manuel3108 manuel3108 deleted the chore/typescript branch December 23, 2024 07:17
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.

[FEATURE] Support for TypeScript AST nodes
10 participants