sveltejs / language-tools

The Svelte Language Server, and official extensions which use it
MIT License
1.18k stars 192 forks source link

VSCode plugin: Syntax highlighting not correctly applied to embedded svelte component in markdown #1094

Open wlach opened 2 years ago

wlach commented 2 years ago

Describe the bug Syntax highlighting not correctly applied to embedded svelte component in markdown when using "Svelte for VSCode"

To Reproduce Steps to reproduce the behavior:

Put the following inside a markdown document:

```svelte
<script>
  import { onMount } from "svelte";

  export let data = undefined;
  let dom_node;

  onMount(() => {
    Plotly.newPlot(dom_node, data, {barmode: 'stack'});
  });
</script>

<div id="plotDiv" bind:this={dom_node}></div>

For example a code snippet that is treated in a way you don't expect.

**Expected behavior**

Expected similar syntax highlighting to what I see when viewing a svelte file with that content.

**Actual behaviour**

Get somewhat incorrect / inconsistent formatting (see screenshots)

**Screenshots**
If applicable, add screenshots to help explain your problem.

View for a svelte component (correct):

![image](https://user-images.githubusercontent.com/20569/125509203-c043194f-bed0-45b5-b059-5889b8179b59.png)

View for a svelte component embedded in markdown (incorrect):

![image](https://user-images.githubusercontent.com/20569/125509521-334a0e8b-2d73-4be1-8b2d-f00143efe4b8.png)

**System (please complete the following information):**
 - OS: Mac and Windows 10
 - IDE: VSCode
 - Plugin/Package: "Svelte for VSCode"

**Additional context**
Add any other context about the problem here.

<!-- Want to help us out? Read this to get started:
https://github.com/sveltejs/language-tools#development
-->
jasonlyu123 commented 2 years ago

Seem like the injection rules here doesn't work in the markdown code block for some reason. We'll see whether that's possible to fix.

wlach commented 2 years ago

I spent a bit of time today trying to debug this. I used this toy example to test:

```svelte
<script>
  var x = 5;
</script>

After giving things slightly more unique names, I found out that vscode's textmate parser seems to be "stuck" in this directive when in markdown:

https://github.com/sveltejs/language-tools/blob/6e0396ca18ea5e7da801468eab35cdef43b3c979/packages/svelte-vscode/syntaxes/svelte.tmLanguage.src.yaml#L467

That is to say, when I'm at `var` inside a `script` block, it still thinks I'm in `meta.tag.start.svelte` (renamed to `meta.tag.start3.svelte` in my local checkout). i.e. the scopes are:

entity.other.attribute-name.svelte meta.attribute.var.svelte meta.tag.start3.svelte meta.script.svelte meta.embedded.block.svelte markup.fenced_code.block.markdown text.html.markdown


What's odd is when I put the cursor at the very end of the script block above (i.e. just after the `>`), the textmate scopes are:

punctuation.definition.tag.end.svelte meta.tag.start3.svelte meta.script.svelte source.svelte



So it's noticing the end of the block, but somehow not transitioning into the state where it's actually looking at the JavaScript. I assume that's:

https://github.com/sveltejs/language-tools/blob/6e0396ca18ea5e7da801468eab35cdef43b3c979/packages/svelte-vscode/syntaxes/svelte.tmLanguage.src.yaml#L18

Why would it be triggering `endCaptures` but not actually ending the block? It's very strange.
wlach commented 2 years ago

Had the bright idea of using git bisect, which revealed that this used to work (when it was added in July 2020 with #301) but seems to have been broken by #657

wlach commented 2 years ago

@Monkatraz any ideas on ^^^ ?

wlach commented 2 years ago

Apologies for my flailing around. Dug into things more and realized that @jasonlyu123 was correct all along: the injection rules are not being applied in an embedded context. The weird behaviour I'm describing above is only due to the fact that the injection rules are not there, and it's falling back to whatever else is defined in the file.

I'm now pretty sure this is an issue upstream so filed an issue there: https://github.com/microsoft/vscode-textmate/issues/152 -- that has some more details as well as confirmation of the problem described.

It's likely possible to make the grammar not depend on injections (e.g. I got it working for bare script tags without them), though I think it would be less elegant.

dummdidumm commented 2 years ago

Thank you for digging into this!

wlach commented 2 years ago

Based on the response in https://github.com/microsoft/vscode-textmate/issues/152, it looks like we might have to rewrite this not to use injection grammars (at least if we care about fixing this bug).

Monkatraz commented 2 years ago

Doing that will be horribly messy... How frustrating.

dummdidumm commented 2 years ago

At least this comment gave me another possibility on how to better debug grammar matches

Monkatraz commented 2 years ago

Okay I guess I need to do three things sooner or later:

I've been working with CodeMirror 6 a lot recently and the highlighting there is so much better it's jarring going back to VSCode lol (I should make a Svelte grammar for it)

Monkatraz commented 2 years ago

Although I should add that one alternative is to use a different grammar for markdown, I think one that always uses TypeScript would be a decent compromise. Obviously that has edge cases (what if you're using something nutty like CoffeeScript or Rescript)

dummdidumm commented 2 years ago

This sounds like a decent short-time solution. It would solve the issue for the majority of users, for those drinking coffee or otherwise it's still as buggy as before inside the script tag.

karmaral commented 2 years ago

I'm having a similar issue, but with strings inside html tags. It's pretty annoying. Is there some sort of workaround that can be applied locally? image Something's up with stuff inside the brackets too. It as if the brackets inherit the string color or something.

If it's of any help here are some token inspector captures:

image

image

karmaral commented 2 years ago

I just wanted to update that doing a clean vscode install seems to have fixed my issue. I couldn't backtrack it to something specific though. It wasn't my settings.json nor my extensions, but something in the programs settings UI. It probably too buried to bother looking. image

wlach commented 2 years ago

Although I should add that one alternative is to use a different grammar for markdown, I think one that always uses TypeScript would be a decent compromise. Obviously that has edge cases (what if you're using something nutty like CoffeeScript or Rescript)

Not sure if this is what you had in mind, but I tried something like this in https://github.com/sveltejs/language-tools/pull/1537. Basically reinjecting enough rules to make script/style tags do something sensible if they're embedded in markdown.

dummdidumm commented 2 years ago

Mostly fixed by #1537 through adding additional injection rules for markdown files. I'll keep this open, maybe we find a way someday to solve this in a way that these addition injections are not needed.