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

Check if bundle is valid before restarting #3066

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vinistock
Copy link
Member

@vinistock vinistock commented Jan 15, 2025

Motivation

The more we restart the server, the higher the risk that something related to Bundler may fail and prevent users from having a smooth experience.

I think we can be a bit smarter about when we restart the server by checking if composing the bundle is successful before we try. If we know that composing fails, it's better to continue running the current version of the server than crashing.

Implementation

The proposal is to add a custom request that allows the client to ask the server if composing the bundle succeeds. That way, if there's a lockfile update, we can first check if the update produces a valid bundle.

If we couldn't compose the bundle, then we avoid restarting. Otherwise, we trigger the restart, but skip composing the bundle since we just did that ahead of time - saving time for the user and allow them to skip directly to indexing.

Note

This solution doesn't fully solve #1458, but it does move the needle a little bit at least for VS Code. The big challenge in moving lockfile watching to the server is how to transfer the state of the server currently running to the new instance we launch.

For example, when we receive a notification that the lockfile was changed, we would need to:

  1. Compose the new bundle
  2. Replace the current process with a new one, using the new bundle (via exec)
  3. Transfer the state of the previous process to the new one. We need all configurations that were applied during initialize (think the global state), the state of all current documents (the store object) and other pieces of state that exist in the version of the server running before the relaunch

I have not yet figured out how to achieve 3.

Automated Tests

Added tests.

@vinistock vinistock added the bugfix This PR will fix an existing bug label Jan 15, 2025 — with Graphite App
Copy link
Member Author

vinistock commented Jan 15, 2025

@vinistock vinistock removed the bugfix This PR will fix an existing bug label Jan 15, 2025
@vinistock vinistock added enhancement New feature or request vscode This pull request should be included in the VS Code extension's release notes labels Jan 15, 2025 — with Graphite App
@vinistock vinistock requested review from andyw8 and st0012 January 15, 2025 16:22
@vinistock vinistock marked this pull request as ready for review January 15, 2025 16:26
@vinistock vinistock requested a review from a team as a code owner January 15, 2025 16:26
@vinistock vinistock force-pushed the 01-14-check_if_bundle_is_valid_before_restarting branch from 9b190ed to d123ac4 Compare January 15, 2025 16:27
@vinistock vinistock changed the base branch from 01-15-use_local_executables_when_working_on_the_ruby_lsp to graphite-base/3066 January 15, 2025 19:04
@vinistock vinistock force-pushed the 01-14-check_if_bundle_is_valid_before_restarting branch from d123ac4 to 12d6d3b Compare January 15, 2025 19:04
@vinistock vinistock changed the base branch from graphite-base/3066 to main January 15, 2025 19:05
@vinistock vinistock force-pushed the 01-14-check_if_bundle_is_valid_before_restarting branch 2 times, most recently from e2346fd to ea13ed2 Compare January 16, 2025 16:02
@vinistock vinistock force-pushed the 01-14-check_if_bundle_is_valid_before_restarting branch from ea13ed2 to adf88f7 Compare January 17, 2025 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request vscode This pull request should be included in the VS Code extension's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant