You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR changes the path of the named pipe / domain socket used to communicate between the language client and server to include the version string of rome_service. This ensures a given instance of the Rome CLI binary always spawns (and connects to) an instance of the Rome daemon it's compatible with.
Due to the nature of this check (using filesystem paths) the comparison between version IDs is a strict equality, however as the version string of rome_service is an internal value we could change it independently from the main Rome version number. We could also truncate the patch number from the version string, but for now I expect even patch release of Rome may include breaking changes to the protocol so I'm leaving the full version string.
Test Plan
I tested this locally by running two versions of the Rome server in parallel (the prebuilt binary from the npm package and a locally built version of this branch) and ensuring each version of the CLI and editor extension were connection to the right version.
The rage command to this point discovered any running rome server. It allows to pull the logs from the LSP the VS Code extension is connected even if they don't have the same version (and prints the server version). That will no longer work after this change.
Should we extend the rage command to connect to any server handle?
The rage command to this point discovered any running rome server. It allows to pull the logs from the LSP the VS Code extension is connected even if they don't have the same version (and prints the server version). That will no longer work after this change.
Should we extend the rage command to connect to any server handle?
This would need to detect all the active daemon instances somehow, either by maintaining an explicit list of past rome_service versions and trying to connect to each (not forward compatible) or by enumerating the active sockets (I'd need to look up a way to enumerate the active named pipes in Windows, it might require some unsafe direct calls to the Windows API)
The rage command to this point discovered any running rome server. It allows to pull the logs from the LSP the VS Code extension is connected even if they don't have the same version (and prints the server version). That will no longer work after this change.
Should we extend the rage command to connect to any server handle?
This would need to detect all the active daemon instances somehow, either by maintaining an explicit list of past rome_service versions and trying to connect to each (not forward compatible) or by enumerating the active sockets (I'd need to look up a way to enumerate the active named pipes in Windows, it might require some unsafe direct calls to the Windows API)
I think it would be sufficient for now to iterate over all files and match by name, print the total numbers of running services and connect to the first.
* upstream/main:
fix(npm/js-api): Import type from @rometools/backend-jsonrpc (rome#3647)
doc(npm/js-api): Add experimental warning to README
fix(npm/js-api): Lazy load backend implementations (rome#3652)
feat(rome_lsp): stop the daemon after the last client disconnects (rome#3643)
fix(npm/js_api): Ensure JS API build contains `dist` folder (rome#3648)
fix(rome_cli): ensures the service only connects to compatible versions (rome#3642)
feat(playground): add settings button and group IR (rome#3559)
release: v10.0.1 (rome#3649)
fix(rome_js_analyze): False positives for interactive elements in `useKeyWithClickEvents` (rome#3644)
fix(rome_js_semantic): support for object and array bindings when exporting (rome#3620)
doc(editors): Clarify Rome discovery (rome#3639)
fix(rome_js_analyze): improve the detection of `ReactDOM.render` calls in `noRenderReturnValue` (rome#3626)
chore(npm): add license (rome#3629)
Add rustdocs index
Add environment
Change rust docs to use Netlify
fix(rome_js_analyze): useValidAnchor considering all possible expressions (rome#3599)
fix(rome_js_formatter): Trailing comma inside import rome#3600 (rome#3624)
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #3637
This PR changes the path of the named pipe / domain socket used to communicate between the language client and server to include the version string of
rome_service. This ensures a given instance of the Rome CLI binary always spawns (and connects to) an instance of the Rome daemon it's compatible with.Due to the nature of this check (using filesystem paths) the comparison between version IDs is a strict equality, however as the version string of
rome_serviceis an internal value we could change it independently from the main Rome version number. We could also truncate the patch number from the version string, but for now I expect even patch release of Rome may include breaking changes to the protocol so I'm leaving the full version string.Test Plan
I tested this locally by running two versions of the Rome server in parallel (the prebuilt binary from the npm package and a locally built version of this branch) and ensuring each version of the CLI and editor extension were connection to the right version.