-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix: switch to our fork acorn typescript plugin #15393
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
Conversation
🦋 Changeset detectedLatest commit: b1877db The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
@@ -28,7 +28,7 @@ | |||
{ | |||
"type": "Identifier", | |||
"start": 43, | |||
"end": 46, | |||
"end": 54, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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:
end
of aPattern
now also includes the type annotation, i.e.const msg: boolean = true
will not haveend: 9
butend: 18