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

Consider switching to a more mature parser #13439

Open
ottomated opened this issue Sep 29, 2024 · 12 comments
Open

Consider switching to a more mature parser #13439

ottomated opened this issue Sep 29, 2024 · 12 comments
Labels
compiler Changes relating to the compiler needs discussion
Milestone

Comments

@ottomated
Copy link
Contributor

Describe the problem

There are many issues that have acorn-typescript as the upstream cause (#13179, #13125, #13409, #13188, more), but acorn-typescript is abandoned. It also has several open issues that would need to be addressed once forked or collaboration access is granted.

Describe the proposed solution

One of the following:

  • Fork acorn-typescript. The svelte team would become the primary maintainers but it wouldn't require any changes to the svelte source code.
  • Use an alternate parser, such as swc or oxc. SWC seems to use the same AST types as acorn, but oxc advertises faster speeds. These parsers are much more mature than acorn-typescript and are obviously actively maintained. (note: oxc would require changes to both esrap and the svelte source)

My vote would be for the latter. The downsides (bigger dependencies and changes to svelte's source) are outweighed by the long-term advantage of using a more reliable library.

Importance

nice to have

@dummdidumm
Copy link
Member

We'll fork in the short term but keep an eye out for how the other variants mature over time. The main blocker for adopting other parsers has been that AFAIK no other has a feature like Acorn's "parse expression", which we need for JS-like snippets within the template.

@ottomated
Copy link
Contributor Author

ottomated commented Sep 29, 2024

Looking into this. SWC doesn't expose a function like that but their Rust API exposes a parse_epxr() function that can take something like () => foo++}>increment</button> and spit out an AST (below).

This is what you need, right? In order to make this work, it would take a custom rust package that imports swc_ecma_parser and exposes the custom methods you need to call from JS. This could either be wasm (probably easier) or node bindings (probably faster, but requires different binaries for each platform).

{
  "type": "ArrowFunctionExpression",
  "span": {
    "start": 1,
    "end": 12
  },
  "ctxt": 0,
  "params": [],
  "body": {
    "type": "UpdateExpression",
    "span": {
      "start": 7,
      "end": 12
    },
    "operator": "++",
    "prefix": false,
    "argument": {
      "type": "Identifier",
      "span": {
        "start": 7,
        "end": 10
      },
      "ctxt": 0,
      "value": "foo",
      "optional": false
    }
  },
  "async": false,
  "generator": false,
  "typeParameters": null,
  "returnType": null
}

@ottomated
Copy link
Contributor Author

SWC also exposes a print method that could be used instead of esrap!

@ottomated ottomated mentioned this issue Sep 30, 2024
5 tasks
@ottomated
Copy link
Contributor Author

Did research into this in #13444. It's definitely doable and worth the effort in my opinion but I don't know if it's worth targeting 5.0.

@ottomated
Copy link
Contributor Author

Looked into oxc as well. They also have a parse_expression method, and another advantage is they expose a way to directly transform a typescript AST to a javascript AST - this would probably fix #11502. They don't have a compatibility layer with acorn though.

@dummdidumm
Copy link
Member

dummdidumm commented Sep 30, 2024

Thank you for exploring this! We won't be able to incorporate this into the compiler in the short term. We gotta sit down and think through all the implications (the increased size, if it's more performant or actually worse because of having to cross the bridge all the time etc), then integrate it, which will take more time than what we have left until the release. So this is more of a "look into it for the next major version" thing - therefore giving it the 6.0 milestone (in the sense of, revisit it then, not in the sense of "will happen"). Until then, by all means continue exploring 🙏

@dummdidumm dummdidumm added this to the 6.0 milestone Sep 30, 2024
@dummdidumm dummdidumm added compiler Changes relating to the compiler needs discussion labels Sep 30, 2024
@benmccann
Copy link
Member

I think if we were going to adopt a non-JS parser, the compelling thing about OXC is that Svelte projects are primarily built with Vite and Vite will be using OXC before long via rolldown. Another consideration with non-JS parsers though is that they would need to be compiled to WASM to ship them to the REPL/playground and that would slow down the load of those sites. Though that may be inevitable as Rollup, etc. are going the non-JS route.

I also wonder if we could add a parseExpression method to an existing parser. I don't have that much experience with parsers, so could be naively wrong about this, but that sounds pretty easy. Any parser must already be capable of parsing expressions as part of parsing a file, so it just seems like a small refactoring. I think @typescript-eslint/typescript-estree may be one of the more used pure-JS TypeScript parsers and I think it's acorn-based. While I doubt they have a need for a parseExpression method perhaps they would be amenable to adding one if it meant the Svelte team were also using the library and thus invested in maintaining it.

@ottomated
Copy link
Contributor Author

@benmccann Yep I've implemented an oxc parser in #13444 and I think it's the right move. Currently I've had to patch in a few methods that svelte needs but through talking with the oxc team they'll probably be happy to expose them as well. I've experienced between a 5-50x speed increase in parsing typescript even with the FFI overhead. I'm currently contributing some PRs to oxc with the goal of getting full estree compatibility and typedefs.

@sahinvardar
Copy link

sahinvardar commented Dec 16, 2024

I came across this while working on a Svelte 5 AST printer. While analyzing how Svelte 5 handles TypeScript, I noticed the use of acorn-typescript. However, it seems to have some flaws and appears to be abandoned.

Are there specific reasons why @typescript-eslint/parser isn’t being used instead?
It is actively maintained, thoroughly tested, and fully supports the TSESTree spec. In the best-case scenario, it could serve as a drop-in replacement for acorn-typescript.

If the concern is that we also need an ESTree-compatible printer with TypeScript support, I’ve developed a generator for astring that adds TypeScript support.

@dominikg

This comment was marked as off-topic.

@sahinvardar
Copy link

@dominikg sorry was not meant to be spamming, and the comment by itself is not really off topic. Just used an LLM to have correct gramma and better sentence structure. Sorry about the confusion.

But still curious and maybe make sense, just wanted to know if that would be an viable option.

Are there specific reasons why @typescript-eslint/parser isn’t being used instead?

@dominikg
Copy link
Member

apologies, i misread your comment and unhided it.

@typescript-eslint/parser is huge in comparison and has a lot more dependencies.

https://pkg-size.dev/@typescript-eslint%2Fparser / https://npmgraph.js.org/?q=%40typescript-eslint%2Fparser

vs

https://pkg-size.dev/acorn-typescript / https://npmgraph.js.org/?q=acorn-typescript

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler Changes relating to the compiler needs discussion
Projects
None yet
Development

No branches or pull requests

5 participants