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

Semantic tokens for typescript #2802

Merged
merged 10 commits into from
Apr 8, 2021
Merged

Conversation

jasonlyu123
Copy link
Contributor

@jasonlyu123 jasonlyu123 commented Mar 23, 2021

Fixed #1904
Fixed #2687
Fixed #2434
Fixed #2687

implentation

Add semantic token feature for script region. Due to typescript's implementation. It's currently not available for js. Also because of that, this is currently not available in interpolation mode. But I still left some merging strategy in the project service for potential future implementation.

A couple of things like to discuss.

  1. There's some new built-in vscode command in 1.53
    Wondering if I should update the integration test to use it. The advantage is it's more integrated than a language-server request. And I don't have to copy the semantic token legend to the test script. A disadvantage is we might accidentally ship features not compatible with 1.52.

  2. Computed member is highlighted as a method. Should we need to override it?

screenshot

Here is some screenshot comparison. The theme is GitHub dark.
Without semantic tokens
圖片

With semantic tokens
圖片

Without
圖片

With
圖片

@yoyo930021
Copy link
Member

There's some new built-in vscode command in 1.53
Wondering if I should update the integration test to use it. The advantage is it's more integrated than a language-server request. And I don't have to copy the semantic token legend to the test script. A disadvantage is we might accidentally ship features not compatible with 1.52.

My understanding is that you can use them.
Testing theoretically does not affect users.

Computed member is highlighted as a method. Should we need to override it?

Do you know what it looks like in a typescript file?

@jasonlyu123
Copy link
Contributor Author

jasonlyu123 commented Mar 24, 2021

My understanding is that you can use them.
Testing theoretically does not affect users.

Yes. It's just a very little chance that something doesn't work on an older version and we tested in a newer version so we didn't know. If you think it's ok I'll bump it.

Do you know what it looks like in a typescript file?

Should be the same as my current implementation. I haven't customized it. Because computed properties are method in the option object it has been highlighted as method not property.

@yoyo930021
Copy link
Member

Yes. It's just a very little chance that something doesn't work on an older version and we tested in a newer version so we didn't know. If you think it's ok I'll bump it.

You can bump vscode version in the test. 👍

Should be the same as my current implementation. I haven't customized it. Because computed properties are method in the option object it has been highlighted as method not property.

If same, I think we can keep it. Get feedback after the release.
Consider doing it together with this #2434 .

@jasonlyu123
Copy link
Contributor Author

added #2434. Currently settle with a fallback TextMate scope of entity.name.function. If anyone can think of another better highlight scope, I can change it.

You can also customize the color follow this doc after it goes live.

@jasonlyu123 jasonlyu123 marked this pull request as ready for review March 24, 2021 10:11
@yoyo930021 yoyo930021 self-assigned this Mar 26, 2021
@yoyo930021
Copy link
Member

I will take a look on weekend.

Copy link
Member

@yoyo930021 yoyo930021 left a comment

Choose a reason for hiding this comment

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

LGTM
Sorry for the late reply.

server/src/services/vls.ts Outdated Show resolved Hide resolved
server/src/services/projectService.ts Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
Copy link
Member

@yoyo930021 yoyo930021 left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants