Add LSP functionality to Rover #2272

jonathanrainer · 2024-11-24T08:51:39Z

Now that the LSP has been finalised, and composition has been refactored such that it can consume events in the same way that rover dev does we can unify the two together.

This has been tested by running a custom build version of Rover against VSCode and it seems to function as intended. We will need to hash out a few more things in the review overall but this is a very positive step forwards

svc-apollo-docs · 2024-11-24T08:51:42Z

✅ Docs Preview Ready

No new or changed pages found.

crates/robot-panic/src/lib.rs

crates/rover-std/Cargo.toml

src/command/lsp/README.md

src/command/lsp/input.txt

src/command/lsp/mod.rs

jonathanrainer · 2024-11-25T09:12:32Z

src/command/lsp/mod.rs

+        // TODO: Let the supergraph binary exist inside its own task that can respond to being re-installed etc.
+        let supergraph_binary =
+            InstallSupergraph::new(federation_version.clone(), client_config.clone())
+                .install(
+                    None,
+                    lsp_opts.plugin_opts.elv2_license_accepter,
+                    lsp_opts.plugin_opts.skip_update,
+                )
+                .await?;


One of the features (I think) of the old PR is that it allows people to update the version of Federation inside the supergraph.yaml while it's running. When doing the dev refactor we made an explicit decision that we didn't want to support that behaviour. Is that a dealbreaker for the LSP? Or could we ship this as is and see if there's sufficient demand after the fact to implement that?

It's a pretty bad UX to not watch supergraph.yaml because the way to manually restart the language server underneath VS Code is really clunky—and it's not at all obvious when you should do that. If someone modifies their supergraph.yaml file and still sees out of date validations, it's going to be frustrating.

src/command/lsp/mod.rs

jonathanrainer · 2024-11-25T09:17:26Z

src/command/lsp/mod.rs

+                CompositionEvent::SubgraphAdded(CompositionSubgraphAdded {
+                    name,
+                    schema_source,
+                }) => {
+                    debug!("Subgraph {} added", name);
+                    language_server.add_subgraph(name, schema_source).await;
+                }
+                CompositionEvent::SubgraphRemoved(CompositionSubgraphRemoved { name }) => {
+                    debug!("Subgraph {} removed", name);
+                    language_server.remove_subgraph(&name).await;
+                }


These are net new events because the language server needs to know when subgraphs are added and removed, so I expanded out the definition of CompositionEvents to include these.

trying to get an initial review out, but self-note: probs need to check that all the places we add/remove subgraphs are emitting the right event

src/composition/mod.rs

src/composition/supergraph/config/resolve/mod.rs

jonathanrainer · 2024-11-25T09:20:17Z

src/composition/supergraph/config/resolve/subgraph.rs

+        /// The result of joining the paths together, that caused the failure
+        joined_path: PathBuf,


Found the need to expand the definition of this error to aid my own debugging

src/composition/supergraph/config/resolve/subgraph.rs

src/composition/watchers/composition.rs

src/utils/supergraph_config.rs

jonathanrainer · 2024-11-25T09:28:56Z

Also a general comment:

@dylan-apollo I couldn't see a way to inject a profile into the Rover invocation for the LSP, which meant I had to mess around to get my default profile set up. Is this intentional or did I miss something?

src/cli.rs

src/command/lsp/README.md

src/command/lsp/mod.rs

aaronArinder · 2024-12-02T16:38:35Z

src/command/lsp/mod.rs

+        Config {
+            root_uri: supergraph_content_root.to_string(),
+            enable_auto_composition: false,
+            force_federation: false,
+            disable_telemetry: false,
+        },


I wonder if we should do something else with these options. Eg, the disable_telemetry should probably follow APOLLO_TELEMETRY_DISABLED, but I don't really understand force_federation's use or when you'd want to enable or diasble auto-composition (I guess if you're not relying on rover's composition pipeline?)

src/composition/supergraph/config/resolve/mod.rs

src/composition/supergraph/config/resolve/subgraph.rs

dylan-apollo · 2024-12-02T19:05:44Z

@dylan-apollo I couldn't see a way to inject a profile into the Rover invocation for the LSP, which meant I had to mess around to get my default profile set up. Is this intentional or did I miss something?

@jonathanrainer there is an extraArgs that can be put in the VS Code extension's config to pass a list of arbitrary extra things to Rover... like profile. That's really a catch-all workaround right now, though, we should probably make profile it's own, top-level option in the extension's config.

aaronArinder

slapping a like & subscribe on this bad boy because I think it's mergeable as-is, though I know you'll make certain changes here and there (request re-review if you want); I haven't actually tried this out in the wild, though, but not sure that's really on our team to do

src/composition/mod.rs

src/command/lsp/mod.rs

dotdat · 2024-12-06T21:49:28Z

src/command/lsp/mod.rs

+        }
+    });
+
+    // Early return if there is no `supergraph.yaml` given as there is no further need to construct


I'm a little confused about the early return comment here. Did something change?

Not really, I'd misunderstood something in the original semantics that Dylan had implemented so this restores it. Basically if a user doesn't supply a supergraph.yaml we still want to start the LSP up but we don't need the ability to recompose so there's no need to start the composition watcher stuff. It was implemented a bit differently previously but the semantics are now the same. Have a look at Dylan's PR if you want to compare/contrast.

dotdat · 2024-12-06T21:51:35Z

src/command/lsp/mod.rs

+                    lazily_resolved_supergraph_config
+                        .subgraphs()
+                        .iter()
+                        .map(|(a, b)| (a.to_string(), b.schema().clone())),


nit: would .cloned() work here instead of map?

Had a fiddle around and I don't think so unless I'm missing something obvious, if you want to craft something then feel free to have a go :)

aaronArinder

seems good to me!

src/command/lsp/mod.rs

Clumps together other minor fixes without them dirtying other bits of the commit history.

Takes the approach of dumping the existing LSP code straight into what we have and integrating it to the point of compilation.

We need to emit extra events here such that the LSP can react to them

At present, we only support 2 composition output targets, this adds a third, which means that the output of composition only exists in memory. This means we don't have to write/manage temporary files, and we don't compete with the LSP for usage of stdout.

jonathanrainer requested a review from a team as a code owner November 24, 2024 08:51

jonathanrainer force-pushed the jr/task/ROVER-245 branch 2 times, most recently from 9d740fa to 91bbdc6 Compare November 24, 2024 08:54