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 7.0.0 to adopt LSP 3.16 features #1894

Merged
merged 1 commit into from
Apr 29, 2021

Conversation

testforstephen
Copy link
Collaborator

@testforstephen testforstephen commented Apr 23, 2021

Signed-off-by: Jinbo Wang [email protected]

See language client 7.0 changelog.
split code into common, node and browser to allow using the LSP client and server npm modules in a Web browser via webpack. This is a breaking change and might lead to compile / runtime errors if not adopted.

[email protected] has a breaking change that splits the "LanguageClient" model to "vscode-languageclient/node" subfolder. So we have to update all imports of LanguageClient.

@@ -95,39 +95,39 @@ export interface EventNotification {
}

export namespace StatusNotification {
export const type = new NotificationType<StatusReport, void >('language/status');
export const type = new NotificationType<StatusReport>('language/status');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

export declare class NotificationType<P> extends AbstractMessageSignature {
    private _parameterStructures;
    /**
     * Clients must not use this property. It is here to ensure correct typing.
     */
    readonly _: [P, _EM] | undefined;
    constructor(method: string, _parameterStructures?: ParameterStructures);
    get parameterStructures(): ParameterStructures;
}

See the definition of NotificationType, it only accepts one Generic Type Parameter. No idea why it doesn't report compilation errors before.

}

export namespace ClassFileContentsRequest {
export const type = new RequestType<TextDocumentIdentifier, string, void, void> ('java/classFileContents');
export const type = new RequestType<TextDocumentIdentifier, string, void> ('java/classFileContents');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

export declare class RequestType<P, R, E> extends AbstractMessageSignature {
    private _parameterStructures;
    /**
     * Clients must not use this property. It is here to ensure correct typing.
     */
    readonly _: [P, R, E, _EM] | undefined;
    constructor(method: string, _parameterStructures?: ParameterStructures);
    get parameterStructures(): ParameterStructures;
}

Same as NotificationType, RequestType just accepts 3 generic parameter types.

@fbricon
Copy link
Collaborator

fbricon commented Apr 23, 2021

We have already seen issues with vscode-xml on Theia, because this new dependency requires vscode 1.52, while Theia is only at 1.50 for now

@testforstephen
Copy link
Collaborator Author

vscode-java/package.json

Lines 13 to 15 in f911f00

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

Since the latest vscode-java already requires a minimum 1.53 engine, i assume this won't break Theia.

@fbricon
Copy link
Collaborator

fbricon commented Apr 23, 2021

I prefer certainty over assumptions ;-) @snjeza / @rgrunber please check in Theia/CRW

@rgrunber
Copy link
Member

rgrunber commented Apr 23, 2021

I do get a failure to install 0.77.0 ( which includes bcd7dca#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R14 ) on VSCode 1.50.1 and the marketplace will install a lower version. Is there something particular about Type Hierarchy that forced 1.53 ?

Update : Though I just ran a local Theia instance (@theia/example-browser 1.12.0) with latest vscode-java and it seems to work for me.

Update 2 : Theia will begin to fail with this patch introduced. Looks like the engines/vscode 1.53 requirement doesn't fail Theia. It's the language client moving to 7.0. It's why vscode-java still works, and why vscode-xml started failing.

@testforstephen
Copy link
Collaborator Author

I do get a failure to install 0.77.0 ( which includes bcd7dca#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R14 ) on VSCode 1.50.1 and the marketplace will install a lower version.

If you're in VS Code 1.50.1, VS Code Extensions page only lists those compatible vscode-java versions for you. That means [email protected] (which requires a minimum 1.53 engine) is not visible to those VS Code 1.50.1 users.

Update 2 : Theia will begin to fail with this patch introduced. Looks like the engines/vscode 1.53 requirement doesn't fail Theia. It's the language client moving to 7.0. It's why vscode-java still works, and why vscode-xml started failing.

In Theia Extensions page, what's the highest vscode-java version you can see? Is it 0.77 or a lower compatible version?

@rgrunber
Copy link
Member

rgrunber commented Apr 26, 2021

If you're in VS Code 1.50.1, VS Code Extensions page only lists those compatible vscode-java versions for you. That means [email protected] (which requires a minimum 1.53 engine) is not visible to those VS Code 1.50.1 users.

Yup. VS Code 1.50.1 will not permit the extension to be installed even through local vsix, and installs an earlier version if attempted from the marketplace (eg. 0.76.0 instead of 0.77.0).

In Theia Extensions page, what's the highest vscode-java version you can see? Is it 0.77 or a lower compatible version?

If installing from the extension page for vscode-java, it will install 0.76.0. So on Theia the marketplace also correctly installs the compatible version but if done by "Install from vsix" it doesn't complain, which might be a separate bug.

@testforstephen
Copy link
Collaborator Author

If installing from the extension page for vscode-java, it will install 0.76.0. So on Theia the marketplace also correctly installs the compatible version.

Thank you for the verification. The marketplace is the only official channel for installing Java extension. If it ensures the compatibility check, then it's safe for us to use the current mechanism to adopt new capabilities.

@fbricon
Copy link
Collaborator

fbricon commented Apr 28, 2021

we'll release this in 0.79.0, right after 0.78.0, so we can use Java 16 support in Theia

@fbricon
Copy link
Collaborator

fbricon commented Apr 29, 2021

Now requires #1914

@rgrunber
Copy link
Member

This looks fine to me. I've tested out the code action / resolve code action calls and seems to be working as expected. Additional functionality also seems undisturbed with these changes.

@rgrunber rgrunber merged commit 9f32875 into redhat-developer:master Apr 29, 2021
@testforstephen testforstephen deleted the jinbo_client7 branch April 30, 2021 07:42
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