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

refactor: persist workspaces across connections #5193

Merged
merged 2 commits into from
Feb 25, 2025

Conversation

arendjr
Copy link
Contributor

@arendjr arendjr commented Feb 24, 2025

Summary

Changes the daemon logic to reuse workspaces across connections.

Motivations:

  • This could be a significant break in expectations for some users, even though I expect most to be unaffected. But this means that if we want to change the logic, we better do it before the 2.0 release.
  • I'm preparing to add a filesystem watcher for the workspaces, and if every connection gets its own workspace, that means every connection needs to get its own watcher too. That seems undesirable, because duplicate watchers wouldn't just do duplicate work, they would also increase the open file count, which can be problematic especially on Linux.
  • The code seemed to suggest that not sharing workspaces was an oversight, so given the above, why not fix it now?
  • If we don't do this, why do we allow the CLI to opt into using the daemon at all? Connecting to the daemon from the CLI only makes sense IMO if you get access to shared caches... it doesn't make much sense if you get a fresh workspace.

Note: This only applies to the daemon, so it mainly affects the LSP server which uses it. The above applies when multiple connections from clients are established to the same daemon. CLI sessions don't use the daemon by default and are unaffected unless you explicitly opt-in.

Test Plan

Added test case.

@arendjr arendjr requested review from a team February 24, 2025 15:35
@github-actions github-actions bot added the A-LSP Area: language server protocol label Feb 24, 2025
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.

Thank you; this is a great fix! I would add a changeset because you mentioned that this could be a breakage for some people.

@arendjr arendjr merged commit 14ad3f5 into biomejs:main Feb 25, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LSP Area: language server protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants