Skip to content

Update the protocol to version 3.17 - #350

Merged
CGNonofr merged 13 commits into
mainfrom
update-lsp-version
May 19, 2022
Merged

Update the protocol to version 3.17#350
CGNonofr merged 13 commits into
mainfrom
update-lsp-version

Conversation

@CGNonofr

@CGNonofr CGNonofr commented May 13, 2022

Copy link
Copy Markdown
Collaborator

Notes:

  • InlineValue feature is not yet supported by monaco, and it's not possible to NOT register the feature

@CGNonofr
CGNonofr force-pushed the update-lsp-version branch from 899d325 to b26baf6 Compare May 13, 2022 17:21
@CGNonofr

Copy link
Copy Markdown
Collaborator Author

This code is not tested yet

And I'm not sure about 440140a and why it was required

@kaisalmen

Copy link
Copy Markdown
Collaborator

@CGNonofr Thank you. I will have a look later today.

@CGNonofr
CGNonofr marked this pull request as draft May 16, 2022 14:56
@CGNonofr
CGNonofr marked this pull request as ready for review May 17, 2022 16:52
@CGNonofr

Copy link
Copy Markdown
Collaborator Author

@CGNonofr Thank you. I will have a look later today.

Ok NOW it's tested and ready to be reviewed

@kaisalmen kaisalmen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Comments don't imply required changes, but please look at the null check. If that needs ok, LGTM

export class MonacoLanguageClient extends BaseLanguageClient {

static bypassConversion = (result: any) => result || undefined;
static bypassConversion = (result: any, token?: vscode.CancellationToken) => token != null ? Promise.resolve(result || undefined) : (result || undefined);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do you want to have a strict null check here, then change to !==?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure to understand what you mean, can you provide the code you want to be there?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Shouldn't token != null betoken !== null?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We can replace != null by !== undefined if you want.

I'm just used to using == null because it allows to check for undefined and null, and it's allowed by the standartjs eslint rules.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Do you want me to change it?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is a boolean expression, if token is defined, then it is true: token ? Promise.resolve(result || undefined) : (result || undefined);

I'm not sure to understand what you mean, token is a boolean?

Should we consider investigating the new issue that came up before. WDYT

Are you talking about #351? I don't think it's a new issue, but I'm currently investigating. There is probably no reason for it to block the next release tho

I mean a real new version 0.19.0 or shall we call it 1.0.0?

I never understand why so many libraries are using a version number < 1 even after years (even monaco!).
But it's not a big deal, as you wish!

@kaisalmen kaisalmen May 19, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm not sure to understand what you mean, token is a boolean?

You can rewrite this as such (token is undefined => false or defined => true). That's what I meant:

static bypassConversion = (result: any, token?: vscode.CancellationToken) => {
    if (token) {
        return Promise.resolve(result || undefined);
    } else {
        return (result || undefined);
    }
};

The method signature is identical to:

static bypassConversion = (result: any, token?: vscode.CancellationToken) => token != null ? Promise.resolve(result || undefined) : (result || undefined);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's aim for 1.0.0. webpack example is almost working again. monaco workers are not working properly. Will fix this today. Release today or tomorrow.

@CGNonofr CGNonofr May 19, 2022

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

You can rewrite this as such (token is undefined => false or defined => true)

That's what I meant when talking about if on anything else than boolean, relying on the trythiness of an object and the falsiness of undefined seems wrong and dangerous, no big deal tho since the whole project is done this way

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please press the merge button if you like. 👍

}

protected registerBuiltinFeatures(): void {
super.registerBuiltinFeatures();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Does the base implementation now perform this registration and it is therefore no longer needed here?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ok, I looked at vscode-languageclient\lib\common\client.js and this is done there. Was that an error to register it twicebefore or was that really an change in the lc lib?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I don't really know, it seems there isn't any consequence of registering a feature twice, so I don't know for how long it was there.

I think these features were proposed back then, so we added them and forgot to remove it when they became builtin

@kaisalmen

Copy link
Copy Markdown
Collaborator

@CGNonofr sorry it took a bit longer. The week didn't turn out as planned. 😅

@CGNonofr
CGNonofr merged commit 5f056e2 into main May 19, 2022
@CGNonofr
CGNonofr deleted the update-lsp-version branch May 19, 2022 13:53
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.

2 participants