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(lsp): register formatter dynamically if possible #1590

Merged
merged 7 commits into from
Jan 18, 2024

Conversation

nhedger
Copy link
Member

@nhedger nhedger commented Jan 18, 2024

Summary

This PR changes the way the formatting providers are registered by the LSP to prevent registering a formatter twice.

The problem

Some LSP clients do not support dynamic registration for formatting providers, so in PR #1042 , we explicitly enabled static registration of the formatters. Unfortunately, this caused clients that support dynamic registration to show two formatters (the statically registered one, and the dynamically registered one).

The solution

This PR adds logic to detect whether the clients supports dynamic registration of formatters, and skips the static registration when they do.

Test Plan

  • Ensure only one formatter is registered in VS Code's Format Document with... menu.

Fixes #1584

Copy link

netlify bot commented Jan 18, 2024

Deploy Preview for biomejs ready!

Name Link
🔨 Latest commit d0a4822
🔍 Latest deploy log https://app.netlify.com/sites/biomejs/deploys/65a935121e0c09000821cf69
😎 Deploy Preview https://deploy-preview-1590--biomejs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 94 (🔴 down 6 from production)
Accessibility: 97 (no change from production)
Best Practices: 100 (no change from production)
SEO: 93 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions github-actions bot added the A-LSP Area: language server protocol label Jan 18, 2024
@nhedger nhedger changed the title fix(biome_lsp): use dynamic formatter registration when possible fix(lsp): use dynamic formatter registration when possible Jan 18, 2024
@nhedger nhedger changed the title fix(lsp): use dynamic formatter registration when possible fix(lsp): register formatter dynamically if possible Jan 18, 2024
@nhedger nhedger marked this pull request as ready for review January 18, 2024 14:04
@nhedger nhedger requested review from a team January 18, 2024 14:04
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

It looks great! Can you add a changelog line before merging?

@github-actions github-actions bot added the A-Changelog Area: changelog label Jan 18, 2024
@nhedger
Copy link
Member Author

nhedger commented Jan 18, 2024

It looks great! Can you add a changelog line before merging?

I've added the changelog entry under a new LSP header, hope that makes sense.

CHANGELOG.md Outdated Show resolved Hide resolved
@ematipico
Copy link
Member

Cool, you'll have to run just gen-web, commit and merge :D thank you!

@github-actions github-actions bot added the A-Website Area: website label Jan 18, 2024
@nhedger
Copy link
Member Author

nhedger commented Jan 18, 2024

Thanks for the assist, much appreciated!

@nhedger nhedger merged commit 3b3d2e6 into main Jan 18, 2024
20 checks passed
@nhedger nhedger deleted the fix/multiple-lsp-entries branch January 18, 2024 14:41
ematipico added a commit to DaniGuardiola/biome that referenced this pull request Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-LSP Area: language server protocol A-Website Area: website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Multiple entries in the list of formatters
2 participants