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

Prepare for semantic token based highlighting #523

Merged
merged 3 commits into from
Dec 9, 2020
Merged

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Dec 2, 2020

Dev Environment

I pinned the version to the latest LTS via NVM as instructed.

Dependency Changes

Bumping vscode-languageclient to 7.0.0 enables the client to speak LSP 3.16 (version which introduces semantic tokens), which is still "upcoming" and so that new part of the protocol may still change until it's finally stabilized.

FWIW the official Go VS Code extension also uses 7.0.0 series: https://github.com/golang/vscode-go/blob/master/package.json#L60 and so do some other popular extensions.

I picked the same patch version as the Go extension uses, which raises the VSCode requirement to ^1.46.0. We could in theory bump to any later version, but that also means cutting off any VS Code versions older than 1.51.

The Go extension actually doesn't formally require 1.46 probably because gopls isn't enabled by default and the requirement is imposed by the language client which doesn't engage until LS is enabled. Our extension relies on the language server in many more ways and suggests installation by default, so I'm more tempted to require 1.46 for the extension on Marketplace level, but not doing so would just show error popup mentioning the incompatibility, so it's probably not a big deal.

"engines": {
"vscode": "^1.42.0"
},

All changes to LSP are additional and hence any existing 3.15 and earlier functionality should not be affected.

Grammar Change

The block type token association is being changed to reflect that semantic tokens use custom scoping (with some fallbacks) and the existing scope storage.type isn't available there so language server would be unable to match it which would lead to confusing UX where some block types are coloured differently than others. Here I am therefore changing to one that is available (entity.name.type), which means we can match the style.

Before

Screenshot 2020-12-07 at 09 59 00 Screenshot 2020-12-07 at 09 59 21

After

Screenshot 2020-12-07 at 11 59 14 Screenshot 2020-12-07 at 11 59 30


LS PR

The language server side is still WIP, but can be tried out by compiling hashicorp/terraform-ls#331

@radeksimko radeksimko force-pushed the f-semantic-tokens branch 3 times, most recently from 049982f to e04e36d Compare December 7, 2020 11:01
@radeksimko radeksimko marked this pull request as ready for review December 7, 2020 11:06
@radeksimko radeksimko requested a review from a team December 7, 2020 22:15
package-lock.json Outdated Show resolved Hide resolved
@appilon
Copy link
Contributor

appilon commented Dec 9, 2020

@radeksimko can you rerun the install commands but with node LTS, you can version switch with nvm

➜ nvm install --lts
Installing latest LTS version.
v14.15.1 is already installed.
Now using node v14.15.1 (npm v6.14.8)

@radeksimko
Copy link
Member Author

@appilon Done, PTAL.

@radeksimko radeksimko requested a review from appilon December 9, 2020 10:57
Copy link
Contributor

@appilon appilon left a comment

Choose a reason for hiding this comment

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

I believe the engines vscode version needs a bump to 1.46 also now?

Copy link
Contributor

@appilon appilon left a comment

Choose a reason for hiding this comment

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

My preference is for the types and runtime/marketplace filter to remain consistent, although I do understand in our situation it is technically possible to leave the runtime lower (1.42). We should let @aeschright chime in so we're all on the same page.

@aeschright
Copy link
Contributor

My preference is for the types and runtime/marketplace filter to remain consistent, although I do understand in our situation it is technically possible to leave the runtime lower (1.42). We should let @aeschright chime in so we're all on the same page.

I agree, and given that 1.46 was released in May I think it's safe to assume that anyone using our extension has upgraded at least to that point.

@radeksimko radeksimko merged commit e2185ff into master Dec 9, 2020
@radeksimko radeksimko deleted the f-semantic-tokens branch December 9, 2020 21:16
@ghost
Copy link

ghost commented Jan 8, 2021

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the context necessary to investigate further.

@ghost ghost locked as resolved and limited conversation to collaborators Jan 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants