Skip to content

Conversation

@dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented Feb 26, 2025

Since the original library is abandoned, we had to fork it and continue maintenance there.

This PR switches out the package to https://github.com/sveltejs/acorn-typescript

The two notable changes from this are:

  • no more "fix the TS AST afterwards" walk, might speed up things a tiny bit
  • the end of a Pattern now also includes the type annotation, i.e. const msg: boolean = true will not have end: 9 but end: 18

@changeset-bot
Copy link

changeset-bot bot commented Feb 26, 2025

🦋 Changeset detected

Latest commit: b1877db

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

@github-actions
Copy link
Contributor

Playground

pnpm add https://pkg.pr.new/svelte@15393

@svelte-docs-bot
Copy link

"type": "Identifier",
"start": 43,
"end": 46,
"end": 54,
Copy link
Member Author

Choose a reason for hiding this comment

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

notably, the identifier end position is now including the type annotation, not just the name itself.
Judging from the tests that's not a problem but I wanted to make sure to point this out in case someone thinks this could have bad consequences

Copy link
Member

Choose a reason for hiding this comment

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

It's not just identifiers, it's any Pattern node. Turns out @typescript-eslint/parser does the same thing, though TypeScript keeps the node and the annotation separate

So it looks like there's no universally agreed-upon convention here. I don't see any real downside to doing it this way, so if it's hard to keep things as they were for whatever reason then I think this change is probably fine

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, any Pattern, and yes @typescript-eslint/parser also does it that way (and in fact that one specced the TSEStree which the acorn plugin follows).

The only downside could be someone relying on this somewhere else, but since they switch the library, and since it was broken in many places before, too (which is why we needed that fix-walk) so it should be fine in practise.

I also checked language-tools, and turns out we already had to do "use type annotation start instead of node end because we only want the JS part" in a couple of places already, so no breakage there, either.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tend to keep it like it's now to adhere to the inofficial estree spec - this is good to merge if you agree

@dummdidumm dummdidumm marked this pull request as ready for review March 4, 2025 10:35
@Rich-Harris Rich-Harris merged commit ef7fd96 into main Mar 4, 2025
10 checks passed
@Rich-Harris Rich-Harris deleted the switch-acorn-plugin branch March 4, 2025 13:36
@github-actions github-actions bot mentioned this pull request Mar 4, 2025
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.

3 participants