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

fix: register providers only for languages matching the documentSelector #101

Merged
merged 1 commit into from
Aug 1, 2018

Conversation

gins3000
Copy link
Contributor

@gins3000 gins3000 commented Aug 1, 2018

Previously, providers were registered for every registered language, not just for the ones matching the documentSelector array passed to the LanguageClient config.

With this fix, language-specific providers are only registered for an existing language if there is a chance of matching, i.e.:

  1. the language appears in the documentSelectors list
  2. there is at least one documentSelector with non-language matching criteria

This reduces the risk of a conflict with existing providers, e.g. when using monaco-languages or monaco-json.

Copy link
Contributor

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

Did a quick test, looks good:
screen shot 2018-08-01 at 14 04 18

@@ -492,4 +526,16 @@ export class MonacoLanguages implements Languages {
return selector === model.languageId;
}

protected matchLanguage(selector: string | DocumentFilter | DocumentSelector, languageId: string): boolean {
if (Array.isArray(selector)) {
return selector.findIndex(filter => this.matchLanguage(filter, languageId)) !== -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

fyi: selector.some(...) === selector.findIndex(...) !== -1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I would have used "some", but I saw findIndex used in matchModel and thought it's a coding standard you want to stick to. Will keep in mind for future PRs

Copy link
Contributor

Choose a reason for hiding this comment

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

true, I will change it in: #100

@akosyakov
Copy link
Contributor

akosyakov commented Aug 1, 2018

This reduces the risk of a conflict with existing providers, e.g. when using monaco-languages or monaco-json.

I won't use monaco-json together with json LS. It would be confusing for a user. I like the changes nevertheless.

@akosyakov akosyakov merged commit e2b66a4 into TypeFox:master Aug 1, 2018
@akosyakov
Copy link
Contributor

@gins3000 thanks for your contribution, please ⭐️ the repo 🙏 if you like it :)

@gins3000
Copy link
Contributor Author

gins3000 commented Aug 1, 2018

@akosyakov thanks for the thorough and fast review. I checked, and saw that, indeed, I hadn't starred the repo yet and rectified that ;)

@gins3000 gins3000 deleted the register-less-providers branch August 2, 2018 12:01
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.

None yet

2 participants