Skip to content

(feat) Semantic tokens #752

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

Merged
merged 12 commits into from
Jan 13, 2021
Merged

Conversation

jasonlyu123
Copy link
Member

related to #71

This implements the semantic token for typescript. And because of the typescript implementation, this feature doesn't work with javascript.

I tried to add a special highlight to the event dispatcher function but the event semantic token type is not supported by a lot of themes. It ends up not highlighted. I didn't include it in the PR but maybe we could make it optional and hide it behind a setting so that people with the supported theme can enable it.

The original feature request asks for svelte specific highlight but I didn't think of any svelte specific semantic token type. Most of the special syntax doesn't have much to be semantically analyzed. The action, transition, and animation are all supposed to be a function. What do you guys think? Is there any syntax you would like to be covered in semantic tokens?

Screenshot:

Theme: GitHub dark

Without semantic tokens
圖片
With semantic tokens
圖片

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Very nice! Only thing I gotta check some more is how much it affects performance and how different it looks (in terms of default true or false). We might also think about checking the TypeScript settings and not enable semantic highlighting if the user turned it off for TypeScript, even if it's turned on for Svelte.

About your question: I don't have that much of an opinion on that and I like the current coloring. We might start with what we have here and enhance if there's more feedback.

@dummdidumm
Copy link
Member

Had a close look, seems either my eyes don't see it or my theme doesn't colorize as much, either way there's not much difference although I see some semantic tokens applied when inspecting with the token inspector 😄
I noticed one thing we can do for speedup, see code comment.

@jasonlyu123
Copy link
Member Author

Another problem I forgot to mention is that the source map issue of each block is highlighted by the semantic token. So it does look a little bit odd.
圖片

@dummdidumm
Copy link
Member

dummdidumm commented Jan 12, 2021

I'm okay with this since it's opt-out. We should really tackle this off-by-one-error at some point, if that's even possible. Edit: I see you opoened an issue over at Magic String for this, nice! Edit 2: I think the issue you found is actually correct behavior, but there's still something wrong in my opinion (see the issue for more details).

@Monkatraz
Copy link
Contributor

Monkatraz commented Jan 12, 2021

Just to throw my two cents in about this feature: My personal theme makes heavy use of semantic colors, and I really appreciate the effort undertaken to implement them here.

On a more technical note: Many themes for VSCode are quite old - they don't specify semantic token colors even though they could make the syntax highlighting significantly more consistent or meaningful. A good example would be with const - using semantic tokens, you can tell at a glance which variables are const and which are let. This extends to functions being passed as variables, too.

tldr: on a lot of themes you won't see much of a difference, but on others it's huge

EDIT: here, I got an example!

Off:
image

On:
image

Just for fun, here is what you can notice in that last picture:

  • FaunaDB.Client was recognized as a class, not a type
  • this.client gets colored correctly as a property, not a generic variable
  • Functions of this are colored as functions, even when they're not being called
  • q is being colored as a constant anywhere it's used, which is correct
  • ENV, which is a constant, is being colored as a constant even though it lives in an entirely separate file

@dummdidumm dummdidumm merged commit f9db434 into sveltejs:master Jan 13, 2021
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