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

svelte: Migrate to tree-sitter-grammars/tree-sitter-svelte #17529

Merged
merged 26 commits into from
Sep 17, 2024

Conversation

AlbertMarashi
Copy link
Contributor

@AlbertMarashi AlbertMarashi commented Sep 7, 2024

Note

The https://github.com/tree-sitter-grammars/tree-sitter-svelte repository seems to be more well maintained, with higher quality code, and as per zed-extensions/svelte#1 it was suggested that we swap to this repository for Svelte grammars

image
image

Release Notes:

  • N/A

tree-sitter-grammar things to improve

  • snippet functions aren't being treated as JS code
  • we should be able to detect @component comments and treat them as markdown
  • foo:bar style/class/prop directives
  • --foo="..." var fields
  • snippet/if blocks's children may need to be indented a little further

Will implement some of the rest of these in a separate PR

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Sep 7, 2024
@matubu
Copy link
Contributor

matubu commented Sep 7, 2024

I was also experimenting on improving Svelte support, with improvements to the tree sitter but with a different technique. Instead of trying to reimplement a basic JS parser inside the Svelte tree sitter, I extended the tree-sitter-javascript. It could have the advantage of only doing one pass on the JS expression code, and it also means that the JS parser is identical to the tree sitter parser. Also, errors might be handled better as tree sitter error recovery can work in JS expressions too. Maybe we should collaborate to make the Svelte support better.

I also fixed auto indent issues.

Here is the current result of what I was working on:
image

How it currently looks
image

And how it looks on vscode for reference
image

@AlbertMarashi
Copy link
Contributor Author

AlbertMarashi commented Sep 9, 2024

@matubu I had this idea as well, however I realised it is possible to reliably parse the inside of expressions as raw text - because we end up re-parsing the javascript code as an @content and @language injectable, we don't lose any benefits for syntax highlighting or error handling.

Additionally, as svelte supports typescript within expressions now, it is important to support that, and I think the raw parsing right now is the best way to do that.

I'd be happy to collaborate. Could you explain what you had to do in order to get directive props parsing? Were you using a modified version of tree-sitter-svelte?

--

Anyhow, I think we should 100% stick with improving the tree-sitter-grammars/tree-sitter-svelte repository as it's important to centralise efforts here.

I'll be creating a PR soon to add support for directives within their repository soon.

@AlbertMarashi
Copy link
Contributor Author

AlbertMarashi commented Sep 9, 2024

@matubu in hindsight, you are right about some stuff with the JS parsing needing to be more consistent.

For example, here we have a snippet function's name not being highlighted as a function like is done inside of {@render foo()}

image

I think it may be a wise idea (as a seperate PR, and within the tree-sitter-grammars/tree-sitter-svelte) to propose such a change.

From my understanding, we may be able to somehow import/extend the typescript tree-sitter and use it to parse specific node types more directly.

I think it would also be interesting to get injected CSS syntax highlighting for things like `style="background: red"

@AlbertMarashi AlbertMarashi marked this pull request as ready for review September 9, 2024 08:38
@zed-industries-bot
Copy link

zed-industries-bot commented Sep 9, 2024

Messages
📖

This PR includes links to the following GitHub Issues: #zed-extensions/svelte#1, #17310, #10893, #12833, #14943, #zed-extensions/svelte#2
If this PR aims to close an issue, please include a Closes #ISSUE line at the top of the PR body.

Generated by 🚫 dangerJS against ae6627e

@AlbertMarashi AlbertMarashi changed the title [WIP] Migrating to tree-sitter-grammer's svelte grammar Migrate to new tree-sitter for svelte Sep 9, 2024
@matubu
Copy link
Contributor

matubu commented Sep 9, 2024

@matubu I had this idea as well, however I realised it is possible to reliably parse the inside of expressions as raw text - because we end up re-parsing the javascript code as an @content and @language injectable, we don't lose any benefits for syntax highlighting or error handling.

Well what I thought was if the js expression is not properly balanced tree sitter could maybe better recover from it.

I'd be happy to collaborate. Could you explain what you had to do in order to get directive props parsing? Were you using a modified version of tree-sitter-svelte?

Yes. I think you are required to parse the different part of the directive props to be able to highlighting them differently.

From my understanding, we may be able to somehow import/extend the typescript tree-sitter and use it to parse specific node types more directly.

Yes that should be possible too.

I think it would also be interesting to get injected CSS syntax highlighting for things like `style="background: red"

Yes I've done it in my version.

@AlbertMarashi AlbertMarashi changed the title Migrate to new tree-sitter for svelte Migrate Svelte treesitter: better syntax highlighting, outlines/symbol support, svelte 5 support Sep 11, 2024
@savetheclocktower
Copy link

For example, here we have a snippet function's name not being highlighted as a function like is done inside of {@render foo()}

Since foo in that example is represented by the snippet_name node, you could highlight it like a function pretty easily, right?

@AlbertMarashi
Copy link
Contributor Author

For example, here we have a snippet function's name not being highlighted as a function like is done inside of {@render foo()}

Since foo in that example is represented by the snippet_name node, you could highlight it like a function pretty easily, right?

Good point, I will do that tomorrow, however I do think it still should be turned into an actual "raw_text_expression" or something so it can be properly delegated to the typescript syntax highlighting - which will work better for other stuff like function parameters and etc.

@maxdeviant maxdeviant changed the title Migrate Svelte treesitter: better syntax highlighting, outlines/symbol support, svelte 5 support svelte: Migrate to tree-sitter-grammars/tree-sitter-svelte Sep 12, 2024
We'll bump this separately.
@savetheclocktower
Copy link

savetheclocktower commented Sep 12, 2024

For example, here we have a snippet function's name not being highlighted as a function like is done inside of {@render foo()}

Since foo in that example is represented by the snippet_name node, you could highlight it like a function pretty easily, right?

Good point, I will do that tomorrow, however I do think it still should be turned into an actual "raw_text_expression" or something so it can be properly delegated to the typescript syntax highlighting - which will work better for other stuff like function parameters and etc.

Not quite, because in the case of

{#snippet foo({ bar, baz })}

the section

foo({ bar, baz })

in isolation will be treated like a function call rather than a function definition. That matters for things like tags.scm — a symbols list might skip it if it doesn't think it's a function definition. It also matters for syntax highlighting because some themes (at least in Pulsar, the editor I contribute to) apply different styles to function definitions and function parameters than they do object literals and function calls.

If you want to inject JavaScript into that whole example instead of just the { bar, baz } part, it should still be possible. But this is another scenario like your inline CSS example — what we'd want here is to throw Tree-sitter into a particular context and say “pretend you already know this is a function definition and resume parsing.”

@AlbertMarashi
Copy link
Contributor Author

AlbertMarashi commented Sep 13, 2024

{#snippet outer({ hello, bar )}

image

@savetheclocktower

I'm not sure if I misinterpreted you or if you misinterpreted me, but as you can see above, the snippet statement expression code is treated as svelte tree-sitter nodes (including the ( ) parenthesis), instead of a svelte_raw_text like an {#if expr} does in the condition key below.

image

I think it would be wise to defer the handling of the syntax highlighting here to the typescript tree-sitter - where typescript would treat this as a function call expression (even though it should be a function definition expression)

I would like to work on a larger PR at tree-sitter-svelte at some stage to fully parse every JS/TS expression much like the tsx tree-sitter would to solve some of these issues.

In the meantime, I will just highlight the snippet_name as a function

@savetheclocktower
Copy link

I'm not sure if I misinterpreted you or if you misinterpreted me, but as you can see above, the snippet statement expression code is treated as svelte tree-sitter nodes (including the ( ) parenthesis), instead of a svelte_raw_text like an {#if expr} does in the condition key below.

Sure, I understood that far. I was working from memory and thought that there was a single node that wrapped all of these nodes (from snippet_name all the way through ")":

Screenshot 2024-09-12 at 6 27 18 PM

Since there isn't, what I'm describing isn't quite as easy… but my thought process was that, if there were a node that encompassed all that content, you'd be able to create an injection for it by setting injection.include-children. (Might still be possible to pull it off even as a series of sibling nodes, but I haven't tried.)

My preference would be to create that extra level of hierarchy so that you could decide whether to inject into the whole area (like you want) or just into svelte_raw_text — that'd be the most flexible approach.

I would like to work on a larger PR at tree-sitter-svelte at some stage to fully parse every JS/TS expression much like the tsx tree-sitter would to solve some of these issues.

Yeah, there are a few options here, and they all suck in different ways:

  • Formally extend or reference the JS and TS grammars within tree-sitter-svelte (which would vastly increase the size of the parser, I imagine, but would allow for very granular usage of arbitrary rules)
  • Create one-off grammars based on the JS and TS parsers (respectively) that include just the subset of each parser needed to parse solely inside of a function arguments context (less coupled than the option above, but no less wasteful)
  • Inject the existing JS and TS grammars into these ranges and find a way to re-scope the ranges selectively (“if you're in a svelte snippet injection, scope this as function.definition even though it's a function call,” etc.; hacky and would involve lots of compromises)

@AlbertMarashi
Copy link
Contributor Author

@savetheclocktower we can continue this discussion in tree-sitter-grammars/tree-sitter-svelte#10

Copy link
Member

@maxdeviant maxdeviant left a comment

Choose a reason for hiding this comment

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

Thank you!

@maxdeviant maxdeviant merged commit accff82 into zed-industries:main Sep 17, 2024
8 checks passed
maxdeviant added a commit that referenced this pull request Sep 17, 2024
This PR bumps the Svelte extension to v0.2.0.

Changes:

- #17529

Release Notes:

- N/A
maxdeviant added a commit to zed-extensions/svelte that referenced this pull request Oct 18, 2024
…zed#17529)

> [!NOTE]
> The https://github.com/tree-sitter-grammars/tree-sitter-svelte
repository seems to be more well maintained, with higher quality code,
and as per #1 it was
suggested that we swap to this repository for Svelte grammars

- Closes zed-industries/zed#17310
- Closes zed-industries/zed#10893
- Closes zed-industries/zed#12833
- Closes #1
- Closes zed-industries/zed#14943
- Closes #2

- Added: buffer/file symbol outlines for `.svelte` (`outlines.scm`)
- Improved: Attribute directives & modifiers in `.svelte` files can be
styled independently.
- Fixed: issue where svelte expression inside quotes failed parsing
- Improved: Svelte components in Markup are styled differently from
tags.
- Added: Support for Svelte 5 syntax (`{#snippet children()}`, `{@render
foo()`)
- Change: Svelte now using
[tree-sitter-grammars/tree-sitter-svelte](https://github.com/tree-sitter-grammars/tree-sitter-svelte)
for language highlighting
- Added: Support for typescript syntax in svelte expressions


![image](https://github.com/user-attachments/assets/49d199ee-7550-49a7-912d-070cf691b029)

![image](https://github.com/user-attachments/assets/848ac5b6-62da-4c42-8e24-b7023504f8af)

Release Notes:

- N/A

---

**tree-sitter-grammar things to improve**
- [ ] snippet functions aren't being treated as JS code
- [ ] we should be able to detect @component comments and treat them as
markdown
- [x] `foo:bar` style/class/prop directives
- [x] `--foo="..."` var fields
- [ ] snippet/if blocks's children may need to be indented a little
further

Will implement some of the rest of these in a separate PR

---------

Co-authored-by: Marshall Bowers <[email protected]>
maxdeviant added a commit to zed-extensions/svelte that referenced this pull request Oct 18, 2024
This PR bumps the Svelte extension to v0.2.0.

Changes:

- zed-industries/zed#17529

Release Notes:

- N/A
noaccOS pushed a commit to noaccOS/zed that referenced this pull request Oct 19, 2024
…ustries#17529)

> [!NOTE]
> The https://github.com/tree-sitter-grammars/tree-sitter-svelte
repository seems to be more well maintained, with higher quality code,
and as per zed-extensions/svelte#1 it was
suggested that we swap to this repository for Svelte grammars

- Closes zed-industries#17310
- Closes zed-industries#10893
- Closes zed-industries#12833
- Closes zed-extensions/svelte#1
- Closes zed-industries#14943
- Closes zed-extensions/svelte#2

- Added: buffer/file symbol outlines for `.svelte` (`outlines.scm`)
- Improved: Attribute directives & modifiers in `.svelte` files can be
styled independently.
- Fixed: issue where svelte expression inside quotes failed parsing
- Improved: Svelte components in Markup are styled differently from
tags.
- Added: Support for Svelte 5 syntax (`{#snippet children()}`, `{@render
foo()`)
- Change: Svelte now using
[tree-sitter-grammars/tree-sitter-svelte](https://github.com/tree-sitter-grammars/tree-sitter-svelte)
for language highlighting
- Added: Support for typescript syntax in svelte expressions


![image](https://github.com/user-attachments/assets/49d199ee-7550-49a7-912d-070cf691b029)

![image](https://github.com/user-attachments/assets/848ac5b6-62da-4c42-8e24-b7023504f8af)

Release Notes:

- N/A

---

**tree-sitter-grammar things to improve**
- [ ] snippet functions aren't being treated as JS code
- [ ] we should be able to detect @component comments and treat them as
markdown
- [x] `foo:bar` style/class/prop directives
- [x] `--foo="..."` var fields
- [ ] snippet/if blocks's children may need to be indented a little
further

Will implement some of the rest of these in a separate PR

---------

Co-authored-by: Marshall Bowers <[email protected]>
noaccOS pushed a commit to noaccOS/zed that referenced this pull request Oct 19, 2024
This PR bumps the Svelte extension to v0.2.0.

Changes:

- zed-industries#17529

Release Notes:

- N/A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
6 participants