Skip to content

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.

Sign up for GitHub

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

Merged
Rich-Harris merged 9 commits into from
Mar 4, 2025
Merged

fix: switch to our fork acorn typescript plugin #15393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c1204dc
fix: switch to our fork acorn typescript plugin
dummdidumm Feb 26, 2025
7b06375
changeset
dummdidumm Feb 26, 2025
eeb3c32
bump
dummdidumm Feb 27, 2025
2c62472
bump
dummdidumm Feb 27, 2025
934d1a6
bump
dummdidumm Feb 27, 2025
07e421a
adjust snapshots
dummdidumm Feb 27, 2025
636b9cc
this should be obsolete now
dummdidumm Feb 27, 2025
16431b7
bump
dummdidumm Feb 27, 2025
b1877db
bump
dummdidumm Mar 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/selfish-cougars-allow.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

chore: switch acorn-typescript plugin
2 changes: 1 addition & 1 deletion packages/svelte/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@
"@jridgewell/sourcemap-codec": "^1.5.0",
"@types/estree": "^1.0.5",
"acorn": "^8.12.1",
"acorn-typescript": "^1.4.13",
"@sveltejs/acorn-typescript": "^1.0.5",
"aria-query": "^5.3.1",
"axobject-query": "^4.1.0",
"clsx": "^2.1.1",
Expand Down
47 changes: 2 additions & 45 deletions packages/svelte/src/compiler/phases/1-parse/acorn.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
/** @import { Comment, Program } from 'estree' */
/** @import { Node } from 'acorn' */
import * as acorn from 'acorn';
import { walk } from 'zimmerframe';
import { tsPlugin } from 'acorn-typescript';
import { locator } from '../../state.js';
import { tsPlugin } from '@sveltejs/acorn-typescript';

const ParserWithTS = acorn.Parser.extend(tsPlugin({ allowSatisfies: true }));
const ParserWithTS = acorn.Parser.extend(tsPlugin());

/**
* @param {string} source
Expand Down Expand Up @@ -48,7 +46,6 @@ export function parse(source, typescript, is_script) {
}
}

if (typescript) amend(source, ast);
add_comments(ast);

return /** @type {Program} */ (ast);
Expand All @@ -71,7 +68,6 @@ export function parse_expression_at(source, typescript, index) {
locations: true
});

if (typescript) amend(source, ast);
add_comments(ast);

return ast;
Expand Down Expand Up @@ -173,42 +169,3 @@ function get_comment_handlers(source) {
}
};
}

/**
* Tidy up some stuff left behind by acorn-typescript
* @param {string} source
* @param {Node} node
*/
function amend(source, node) {
return walk(node, null, {
_(node, context) {
// @ts-expect-error
delete node.loc.start.index;
// @ts-expect-error
delete node.loc.end.index;

if (typeof node.loc?.end === 'number') {
const loc = locator(node.loc.end);
if (loc) {
node.loc.end = {
line: loc.line,
column: loc.column
};
}
}

if (
/** @type {any} */ (node).typeAnnotation &&
(node.end === undefined || node.end < node.start)
) {
// i think there might be a bug in acorn-typescript that prevents
// `end` from being assigned when there's a type annotation
let end = /** @type {any} */ (node).typeAnnotation.start;
while (/\s/.test(source[end - 1])) end -= 1;
node.end = end;
}

context.next();
}
});
}
3 changes: 0 additions & 3 deletions packages/svelte/src/compiler/phases/1-parse/ambient.d.ts
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion packages/svelte/tests/parser-modern/samples/comment-before-script/output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@
"id": {
"type": "Identifier",
"start": 52,
"end": 57,
"end": 65,
"loc": {
"start": {
"line": 3,
Expand Down
2 changes: 1 addition & 1 deletion packages/svelte/tests/parser-modern/samples/snippets/output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
{
"type": "Identifier",
"start": 43,
"end": 46,
"end": 54,
Copy link
Member Author

@dummdidumm dummdidumm Feb 27, 2025

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

@Rich-Harris Rich-Harris Feb 27, 2025

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

@dummdidumm dummdidumm Feb 27, 2025

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

@dummdidumm dummdidumm Mar 4, 2025

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

"loc": {
"start": {
"line": 3,
Expand Down
8 changes: 4 additions & 4 deletions packages/svelte/tests/parser-modern/samples/typescript-in-event-handler/output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
"end": 147,
"type": "OnDirective",
"name": "click",
"modifiers": [],
"expression": {
"type": "ArrowFunctionExpression",
"start": 73,
Expand All @@ -48,7 +47,7 @@
{
"type": "Identifier",
"start": 74,
"end": 75,
"end": 87,
"loc": {
"start": {
"line": 6,
Expand Down Expand Up @@ -155,7 +154,7 @@
"id": {
"type": "Identifier",
"start": 102,
"end": 106,
"end": 114,
"loc": {
"start": {
"line": 7,
Expand Down Expand Up @@ -316,7 +315,8 @@
}
]
}
}
},
"modifiers": []
}
],
"fragment": {
Expand Down
1 change: 0 additions & 1 deletion packages/svelte/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
"allowJs": true,
"checkJs": true,
"paths": {
"acorn-typescript": ["./src/compiler/phases/1-parse/ambient.d.ts"],
"svelte": ["./src/index.d.ts"],
"svelte/action": ["./src/action/public.d.ts"],
"svelte/compiler": ["./src/compiler/public.d.ts"],
Expand Down
24 changes: 12 additions & 12 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading