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

Registrer providers using the document selector directly #317

Merged

Conversation

CGNonofr
Copy link
Collaborator

(Proper types will come with new monaco-editor release => see microsoft/monaco-editor#2985)

Some languages servers can handle multiples languages (for instance c and c++) so the document selector contains 2 items.

The current code just uses the selector language and fallback to '*'

There is then 2 issues:

  • If the document selector doesn't contain a language, we need to check the selector at every provider call (it works but it's not ideal)
  • More than 1 provider will be registered for a single language client. When removing the LanguageClient, they will be unregistered 1 by 1 while all are already disposed. Unfortunately, some providers are called every time the provider list of the same type changes, so the soon removed providers will be called a last time and generate an error

@kaisalmen
Copy link
Collaborator

@CGNonofr will this PR be backward compatible with older version monaco-editor or is it dependent on the next release?

@CGNonofr
Copy link
Collaborator Author

I think it already works with versions from 5 years ago. The next release will only contain the right type but the code is already there

@kaisalmen
Copy link
Collaborator

Thank you for the explanation.

@CGNonofr
Copy link
Collaborator Author

CGNonofr commented Mar 1, 2022

@kaisalmen What is the next step for this PR?

@kaisalmen
Copy link
Collaborator

Are you still expecting a review from one of the reviewers? Should I review it?

@CGNonofr
Copy link
Collaborator Author

CGNonofr commented Mar 2, 2022

Usually I ask @mofux but since you are back, why not?

@CGNonofr
Copy link
Collaborator Author

CGNonofr commented Mar 8, 2022

ping?

Copy link
Collaborator

@kaisalmen kaisalmen left a comment

Choose a reason for hiding this comment

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

The changes look good, but the compilation problem needs to be resolved.

}

protected createRangeSemanticTokensProvider(selector: DocumentSelector, provider: DocumentRangeSemanticTokensProvider, legend: SemanticTokensLegend): monaco.languages.DocumentRangeSemanticTokensProvider {
protected createRangeSemanticTokensProvider(provider: DocumentRangeSemanticTokensProvider, legend: SemanticTokensLegend): monaco.languages.DocumentRangeSemanticTokensProvider {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You no longer pass the selector here and in other functions and then the call of this.matchModel(... which does not work. See the following compliation errors:

monaco-languageclient: src/monaco-languages.ts(74,38): error TS2552: Cannot find name 'selector'. Did you mean 'Selection'?
monaco-languageclient: src/monaco-languages.ts(103,38): error TS2552: Cannot find name 'selector'. Did you mean 'Selection'?
monaco-languageclient: src/monaco-languages.ts(124,38): error TS2552: Cannot find name 'selector'. Did you mean 'Selection'?
monaco-languageclient: src/monaco-languages.ts(142,38): error TS2552: Cannot find name 'selector'. Did you mean 'Selection'?
monaco-languageclient: src/monaco-languages.ts(160,38): error TS2552: Cannot find name 'selector'. Did you mean 'Selection'?
monaco-languageclient: src/monaco-languages.ts(178,38): error TS2552: Cannot find name 'selector'. Did you mean 'Selection'?
monaco-languageclient: src/monaco-languages.ts(196,38): error TS2552: Cannot find name 'selector'. Did you mean 'Selection'?
monaco-languageclient: src/monaco-languages.ts(214,38): error TS2552: Cannot find name 'selector'. Did you mean 'Selection'?
monaco-languageclient: src/monaco-languages.ts(241,38): error TS2552: Cannot find name 'selector'. Did you mean 'Selection'?
monaco-languageclient: src/monaco-languages.ts(249,38): error TS2552: Cannot find name 'selector'. Did you mean 'Selection'?
monaco-languageclient: src/monaco-languages.ts(271,38): error TS2304: Cannot find name 'selector'.
monaco-languageclient: src/monaco-languages.ts(289,38): error TS2304: Cannot find name 'selector'.
monaco-languageclient: src/monaco-languages.ts(309,38): error TS2304: Cannot find name 'selector'.
monaco-languageclient: src/monaco-languages.ts(327,38): error TS2304: Cannot find name 'selector'.
monaco-languageclient: src/monaco-languages.ts(345,38): error TS2304: Cannot find name 'selector'.
monaco-languageclient: src/monaco-languages.ts(376,38): error TS2304: Cannot find name 'selector'.
monaco-languageclient: src/monaco-languages.ts(394,38): error TS2304: Cannot find name 'selector'.
monaco-languageclient: src/monaco-languages.ts(412,38): error TS2304: Cannot find name 'selector'.
monaco-languageclient: src/monaco-languages.ts(420,38): error TS2304: Cannot find name 'selector'.
monaco-languageclient: src/monaco-languages.ts(443,38): error TS2304: Cannot find name 'selector'.
monaco-languageclient: src/monaco-languages.ts(467,38): error TS2304: Cannot find name 'selector'.
monaco-languageclient: src/monaco-languages.ts(492,38): error TS2304: Cannot find name 'selector'.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh I'm confused since I manage to publish the @codingame/monaco-languageclient package, I have no idea what happened, it's fixed

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nevermind 🙂

@kaisalmen
Copy link
Collaborator

kaisalmen commented Mar 8, 2022

ping?

@CGNonofr I am sorry, I wrote that comment some days ago, but didn't finish the review process (forgot to press the button), so you were unable to see it.

@CGNonofr CGNonofr force-pushed the fix-provider-registration-document-selector branch from 5711100 to 46a6c2d Compare March 8, 2022 13:25
@kaisalmen
Copy link
Collaborator

LGTM

@CGNonofr
Copy link
Collaborator Author

CGNonofr commented Mar 8, 2022

LGTM

I'll let you merge it/publish then

@kaisalmen kaisalmen merged commit ad43117 into TypeFox:master Mar 8, 2022
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