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

Update languageserver-client #1662

Merged
merged 6 commits into from
Nov 29, 2022
Merged

Update languageserver-client #1662

merged 6 commits into from
Nov 29, 2022

Conversation

alexet
Copy link
Contributor

@alexet alexet commented Oct 26, 2022

This updates languageserver-client allowing for inlay hints (among other things) if codeql supports them. There were a few minor breaking changes so theu are fixed. I also upgraded the jsonrpc dependency so it matches the version to avoid the extra copy.

This does however have a higher vscode dependency (1.67 which is April 2022) so I think I need to bump that as well (I was slightly surprised that npm didn't complain before I did that). However if we wanted to make an upgrade warning that affected the current version then we would need to do it in a release before this.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request. (No user visible changes)
  • Issues have been created for any UI or other user-facing changes made by this pull request. (No user visible changes)
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@alexet alexet requested a review from a team as a code owner October 26, 2022 17:12
Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

Nice. I'm guessing that this doesn't need a corresponding bump in the CLI for any LSP changes there?

Also, we should be careful about breaking users on old versions of vscode. I did a quick look in our telemetry and didn't find anyone on 1.67 or earlier, but our telemetry is incomplete. So, it's probably fine just to issue a error or warning for users on older versions with instructions that they need to either downgrade their extension version or upgrade their vscode version.

Is that something easy to do?

@alexet
Copy link
Contributor Author

alexet commented Oct 27, 2022

We don't need to be careful about CLI version. The CLI will simply report that it know "inlayHints" and LSP will then send it commands if it understands that or ignore it when it doesn't. We could wait until the CLI actually supports this but it shouldn't really matter.

Bumping the vscode requirement in package.json will mean that vscode won't update the extension. If we wanted to do anything it would have to be in the version before the update. Also there doesn't seem to be an api to get the vscode version so we would probably have to do dodgy stuff like seing which methods are in the vscode namespace. The reason I bring this up is that we had some issues where people were confused about why their extension didn't update before we worked out that vscode was from a year ago and it took a while to diagnose.

@aeisenberg
Copy link
Contributor

OK. Looks like we are going to do a vscode release tomorrow West Coast time.

Would you be able to get a PR for whatever changes we need to warn users about vscode version requirements changing? (I'm not really sure what's required, otherwise I could probably do it myself.) And we can make sure that the PR is merged before we do the release tomorrow.

cc: @angelapwen (since I think you will be managing the release.)

@alexet
Copy link
Contributor Author

alexet commented Oct 28, 2022

I don't really know how we would do it. It was purely because it came up recently and aditya suggested a warning so I thought I would point it out that this PR could cause similar issues.

This does nothing until the corresponding CLI code is released, I just needed it for testing and it wasn't blocked on anything. Therefore we aren't really in a rush and this shouldn't block the next release.

We can also make 2 releases in relatively quick succession purely to show a warning as there isn't a huge cost to releasing (or at least there wan't last time I did it).

@angelapwen
Copy link
Contributor

This does nothing until the corresponding CLI code is released, I just needed it for testing and it wasn't blocked on anything. Therefore we aren't really in a rush and this shouldn't block the next release.

Just to clarify, does this mean that I can release later today without this PR merged, and we can release post-Universe with this PR + user warning?

@aeisenberg
Copy link
Contributor

Something like this? #1674

@aeisenberg
Copy link
Contributor

@angelapwen I think you can release today. It's not a requirement to get this PR in for this current release.

@alexet
Copy link
Contributor Author

alexet commented Nov 25, 2022

Ok now we have the warning in, I think this PR should be Ok again, I had quite a painful merge.

Also when I tried to commit in codespaces I got issues with the pre commit hook so this was commited and pushed with --no-verify

@aeisenberg
Copy link
Contributor

Thanks for updating.

Also when I tried to commit in codespaces I got issues with the pre commit hook so this was commited and pushed with --no-verify

This shouldn't be a problem. If your push has any issues, it will be caught in one of the checks.

Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

This looks great (but I haven't tried this out locally).

Can you add a changelog entry?

@alexet
Copy link
Contributor Author

alexet commented Nov 29, 2022

I have added a changelog entry now.

@aeisenberg aeisenberg merged commit 91edad8 into main Nov 29, 2022
@aeisenberg aeisenberg deleted the alexet/update-lsp branch November 29, 2022 18:37
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