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 language-client to 8.1.0 #2377

Merged
merged 6 commits into from
Feb 23, 2023

Conversation

CsCherrYY
Copy link
Contributor

@CsCherrYY CsCherrYY commented Mar 24, 2022

This PR is a part of #2376, to reduce the complexity of that PR. Type Hierarchy support has been introduced since language-client 8.0.0-next.4 so we should update this dependency, the language-client version requires vscode 1.61.0 for compiling, so I update this dependency as well.

This update is incompatible with current Theia. see: eclipse-theia/theia#10993

src/extension.ts Outdated Show resolved Hide resolved
src/hoverAction.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@Eskibear Eskibear left a comment

Choose a reason for hiding this comment

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

LGTM.

@rgrunber rgrunber added the dependencies Pull requests that update a dependency file label May 26, 2022
@rgrunber
Copy link
Member

I ran into a problem updating to the language client to 8.0.1 in vscode-xml, and I suspect it'll be the same here. The extension will break on Theia.

Note: Node 14 needs to be used as with 12, I ran into SyntaxError: Unexpected Token '?'. (nullish coalescing issue).
Afterwards I ran into :

2022-05-26T13:34:36.203Z root INFO [hosted-plugin: 9987] PLUGIN_HOST(9987): PluginManagerExtImpl/loadPlugin(/home/rgrunber/.theia/extensions/vscode-xml-0.21.0.vsix/extension/dist/extension)
2022-05-26T13:34:36.257Z root ERROR [hosted-plugin: 9987] Activating extension 'XML' failed: TypeError: Class extends value undefined is not a constructor or null
    at Object.1365 (/home/rgrunber/.theia/extensions/vscode-xml-0.21.0.vsix/extension/dist/extension.js:2:447499)
    at n (/home/rgrunber/.theia/extensions/vscode-xml-0.21.0.vsix/extension/dist/extension.js:2:655557)
    at Object.71 (/home/rgrunber/.theia/extensions/vscode-xml-0.21.0.vsix/extension/dist/extension.js:2:340748)
    at n (/home/rgrunber/.theia/extensions/vscode-xml-0.21.0.vsix/extension/dist/extension.js:2:655557)
    at Object.4384 (/home/rgrunber/.theia/extensions/vscode-xml-0.21.0.vsix/extension/dist/extension.js:2:307797)
    at n (/home/rgrunber/.theia/extensions/vscode-xml-0.21.0.vsix/extension/dist/extension.js:2:655557)
    at Object.2850 (/home/rgrunber/.theia/extensions/vscode-xml-0.21.0.vsix/extension/dist/extension.js:2:481672)
    at n (/home/rgrunber/.theia/extensions/vscode-xml-0.21.0.vsix/extension/dist/extension.js:2:655557)
    at Object.4060 (/home/rgrunber/.theia/extensions/vscode-xml-0.21.0.vsix/extension/dist/extension.js:2:251645)
    at n (/home/rgrunber/.theia/extensions/vscode-xml-0.21.0.vsix/extension/dist/extension.js:2:655557)

extension.js:2:447499 points to class ProtocolTypeHierarchyItem extends code.TypeHierarchyItem
extension.js:2:251645 points to const vscode_languageclient_1 = __webpack_require__(48);

If I'm understanding how the __webpack_require__ works, const vscode_languageclient_1 is in 47, which gets called at the very beginning by const xmlExtensionApiImplementation_1 = __webpack_require__(47); . This is from xmlExtensionApiImplementation.ts which originates in the vscode-xml sources.

So the act of referencing the language client causes TypeHierarchyItem to be referenced, which Theia does not support

CC'ing @fbricon , @testforstephen in case I'm missing an obvious way to work around this.

@CsCherrYY
Copy link
Contributor Author

@rgrunber Yeah I had opened an issue here: eclipse-theia/theia#10993, it seems Theia should support the related types (e.g., TypeHierarchyItem).

@rgrunber rgrunber linked an issue May 27, 2022 that may be closed by this pull request
@rgrunber
Copy link
Member

We should re-investigate this soon. Once Theia releases, I'm hoping this will work. It seems they recently started supporting the Type Hierarchy API.

https://eclipse-theia.github.io/vscode-theia-comparator/status.html

@CsCherrYY
Copy link
Contributor Author

it seems that Theia has released a new version 1.31.0 which supports TypeHierarchyItem: https://github.com/eclipse-theia/theia/blob/master/CHANGELOG.md#breaking_changes_1.31.0

I will update the PR these days.

@rgrunber
Copy link
Member

I may have been a bit too optimistic. We will still need to wait for the end of the year to be able to make this change, but at least it will happen eventually :(

@CsCherrYY
Copy link
Contributor Author

@rgrunber is there anything I miss about this change?

One concern is that we seem not to have a way to define the required Theia version in package.json, since only the newest Theia supports this API.

@rgrunber rgrunber self-requested a review January 25, 2023 14:09
@CsCherrYY CsCherrYY force-pushed the cs-typehierarchy-lsp branch from 7d6fb9a to 9b1e44f Compare January 30, 2023 07:40
@CsCherrYY CsCherrYY changed the title Update language-client to 8.0.0-next.4 Update language-client to 8.0.2 Jan 30, 2023
@CsCherrYY
Copy link
Contributor Author

CsCherrYY commented Jan 30, 2023

6842d55 updates the language client to newest 8.0.2. Comparing it with the former 8.0.0-next 4, the changes can be found also in https://github.com/microsoft/vscode-languageserver-node#3170-protocol-800-json-rpc-800-client-and-800-server:

  • removeal of onReady()
  • change of ErrorHandler

cc @rgrunber

@CsCherrYY CsCherrYY force-pushed the cs-typehierarchy-lsp branch 2 times, most recently from 8e71654 to 6f6414d Compare January 31, 2023 03:11
@CsCherrYY
Copy link
Contributor Author

maybe eclipse-jdtls/eclipse.jdt.ls#1790 is related. @rgrunber could you provide any comments?

@CsCherrYY
Copy link
Contributor Author

@datho7561 It seems that if we disable the client capability shouldLanguageServerExitOnShutdown, the error notification will not appear and the syntax server can shutdown successfully. Could you help verify it?

image

@rgrunber If it's true, I guess language client 8+ has fixed what we patch in eclipse-jdtls/eclipse.jdt.ls#1790.

@CsCherrYY CsCherrYY force-pushed the cs-typehierarchy-lsp branch from c5afa60 to c7455ae Compare February 13, 2023 05:22
@datho7561
Copy link
Contributor

... if we disable the client capability shouldLanguageServerExitOnShutdown, the error notification will not appear and the syntax server can shutdown successfully

Yes, i think this is the right fix. This is what we did in redhat-developer/vscode-xml@825823c for vscode-xml

Copy link
Contributor

@datho7561 datho7561 left a comment

Choose a reason for hiding this comment

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

After shouldLanguageServerExitOnShutdown is removed/set to false, I think this PR will be good

@@ -99,10 +99,10 @@ function createDocumentSymbolProvider(): DocumentSymbolProvider {
}

if ((<any>symbolResponse[0]).containerName) {
return languageClient.protocol2CodeConverter.asSymbolInformations(<clientSymbolInformation[]>symbolResponse);
return await languageClient.protocol2CodeConverter.asSymbolInformations(<clientSymbolInformation[]>symbolResponse);
Copy link
Contributor

Choose a reason for hiding this comment

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

If the result of the protocol2CodeConverter is a Promise, I think you should be able to return the Promise without using await and it will do the same thing. (this isn't a big deal though).

@datho7561 datho7561 self-requested a review February 14, 2023 13:56
@jdneo
Copy link
Collaborator

jdneo commented Feb 16, 2023

BTW, dose the new version of language client has native support of inlay hint? If yes, maybe we can remove https://github.com/redhat-developer/vscode-java/blob/master/src/inlayHintsProvider.ts and its related code.

@rgrunber rgrunber added this to the Mid March 2023 milestone Feb 16, 2023
@CsCherrYY
Copy link
Contributor Author

@jdneo I guess the anwser is yes. See: https://github.com/microsoft/vscode-languageserver-node#3170-protocol-800-json-rpc-800-client-and-800-server inlay hint support is in the list here. I also find the related types in the language-client library.

@CsCherrYY
Copy link
Contributor Author

it looks like vscode-languageclient version 8.1.0 is available, I'll take a look if we can directly update to this version.

Signed-off-by: Shi Chen <chenshi@microsoft.com>
Signed-off-by: Shi Chen <chenshi@microsoft.com>
Signed-off-by: Shi Chen <chenshi@microsoft.com>
@CsCherrYY CsCherrYY force-pushed the cs-typehierarchy-lsp branch from 7c28e29 to 9c2074c Compare February 17, 2023 02:20
Copy link
Member

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

Overall, looks fine (with the lsp4j 0.20.0 PR in JDT-LS). I tried out rename, codelens, inlay hints, type hierarchy as those seem to be more at risk for breakage. It all seems to work well. The syntax server shuts down on its own as well.

I did noticed : https://github.com/redhat-developer/vscode-java/actions/runs/4200047119/jobs/7285652596#step:9:13 . The build still passes. Is this safe ?

src/extension.ts Outdated
Comment on lines 170 to 171
// https://github.com/redhat-developer/vscode-java/pull/2377#issuecomment-1427268344
shouldLanguageServerExitOnShutdown: false,
Copy link
Member

Choose a reason for hiding this comment

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

Yes, this was fixed in the language client release that supported 3.17. Original issue is at #1928 (comment) . There's no need to set it to false as that's the default value anyways on the server side. In fact, there's a good argument for getting rid of it entirely since it was really to handle some non-protocol behaviour.

@Eskibear
Copy link
Contributor

The build still passes. Is this safe ?

@rgrunber
Probably we should upgrade version of typescript. See microsoft/TypeScript#1778 (comment)

@CsCherrYY CsCherrYY changed the title Update language-client to 8.0.2 Update language-client to 8.1.0 Feb 22, 2023
@rgrunber
Copy link
Member

Ok, then let's go ahead and update typescript here if that will fix it.

Signed-off-by: Shi Chen <chenshi@microsoft.com>
@@ -1397,7 +1397,7 @@
"@types/glob": "5.0.30",
"@types/lodash.findindex": "^4.6.6",
"@types/mocha": "^5.2.5",
"@types/node": "^8.10.51",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since VS Code has switched to node 16 since 1.66 (https://code.visualstudio.com/updates/v1_66#_nodemoduleversion-and-nodejs-api-update), let's update this deps as well. The update will resolve the following error:

node_modules/vscode-jsonrpc/lib/node/main.d.ts(8,37): error TS2307: Cannot find module 'worker_threads' or its corresponding type declarations.

Signed-off-by: Shi Chen <chenshi@microsoft.com>
@CsCherrYY
Copy link
Contributor Author

@rgrunber it looks like that issue got resolved after updating typescript and node :)

@rgrunber rgrunber merged commit 6572351 into redhat-developer:master Feb 23, 2023
@CsCherrYY CsCherrYY deleted the cs-typehierarchy-lsp branch February 23, 2023 04:27
@rgrunber rgrunber removed this from the Mid March 2023 milestone Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support vscode-languageclient@8.1.0
5 participants