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 was archived by the owner on Aug 31, 2023. It is now read-only.
This file contains hidden or 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 adds a new serde_workspace feature to rome_service that automatically derives the Serialize and Deserialize traits from serde on all the types that transit through the Workspace interface (including types declared in other crates and transitively included in this interface). This feature is currently disabled by default but prepares the way for the workspace potentially being callable across IPC or FFI boundaries, and in the meantime still imposes some limitations on how data can transit across the Workspace trait that forces it to work across languages (code actions for instance are now represented as language-agnostic text diffs, instead of language-specific syntax trees)
Test Plan
This code is currently untested since the feature is disabled by default, however since it all relies on the procedural derive macros from serde simply running cargo build -p rome_service --features serde_workspace is enough to verify that everything compiles correctly, maybe this command could be added to the suite of tests being run on GitHub Actions ?
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Does that mean that we limit ourself to a Workspace facade that only uses serializable types? Do you see this as a limiting factor e.g. in a long-running CLI service that isn't restricted by the fact that the returned result types must be serializable? Would it be an option to have a specific Workspace facade for consumers that require a serializable/message based interface or is this something you can still see us do in the future/
I presume that once we start using the new feature internally, we should start seeing its benefits/repercussions?
Besides laying out the groundwork for future work on a client-server system with a shared daemon process, one of the immediate application of this change could be in the JavaScript or WebAssembly bindings. Since this change transitively ensures that most of the "public API" types are serializable it opens the possibility of using serde for transmitting objects across FFI boundaries
Does that mean that we limit ourself to a Workspace facade that only uses serializable types?
Yes that's the general intent for the moment, obviously a lot of concepts for our internal client-server model still need to be fleshed out so the idea I'm currently going with is to have this interface as constrained as possible to facilitate future experimentation.
For instance even in the context of a long running CLI process like monitoring a running bundler development server, I think it could still be beneficial to use a Workspace instance shared with a Language Server process, so that both the editor experience and bundler execution can benefit from hot caches primed by the other process.
In general since the Workspace is a high-level, language-agnostic abstraction most of the data going through this interface is going to be strings intended to be directly displayed to the user, either in the form of console messages or editor UI annotations, so requiring everything to be serializable doesn't seem like a constraint that will be hard to keep
… workspace interface are serializable (rome#2928)
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Labels
None yet
3 participants
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
This PR adds a new
serde_workspacefeature torome_servicethat automatically derives theSerializeandDeserializetraits fromserdeon all the types that transit through theWorkspaceinterface (including types declared in other crates and transitively included in this interface). This feature is currently disabled by default but prepares the way for the workspace potentially being callable across IPC or FFI boundaries, and in the meantime still imposes some limitations on how data can transit across theWorkspacetrait that forces it to work across languages (code actions for instance are now represented as language-agnostic text diffs, instead of language-specific syntax trees)Test Plan
This code is currently untested since the feature is disabled by default, however since it all relies on the procedural derive macros from
serdesimply runningcargo build -p rome_service --features serde_workspaceis enough to verify that everything compiles correctly, maybe this command could be added to the suite of tests being run on GitHub Actions ?