Add a RootsTrackingSupport mixin for servers, use it from DartAnalyzerSupport #85
Conversation
PR HealthChangelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. |
| ); | ||
| } else { | ||
| if (supportsRootsChanged) { | ||
| rootsListChanged!.listen((event) { |
There was a problem hiding this comment.
double checking: does this listener need to be cancelled anywhere?
There was a problem hiding this comment.
The controller for this stream gets cancelled when the client disconnects so no it shouldn't be necessary
| ); | ||
| expect( | ||
| server.roots, | ||
| completion(unorderedEquals([b, c, d])), |
There was a problem hiding this comment.
what happened to root a?
There was a problem hiding this comment.
It gets removed later on line 53 - this is an async expectation for a future that doesn't complete until the completer gets completed on line 55.
There was a problem hiding this comment.
Basically the point of this is to test that if we get a roots changed event while we are currently listing roots, we won't ever surface the intermediate state, we will wait for the nest listRoots call.
| @@ -21,14 +21,21 @@ base mixin DartCliSupport on ToolsSupport, LoggingSupport | |||
| implements ProcessManagerSupport { | |||
| @override | |||
| FutureOr<InitializeResult> initialize(InitializeRequest request) { | |||
| String? unsupportedReason; | |||
| if (request.capabilities.roots == null) { | |||
There was a problem hiding this comment.
nit: since each tool specifies roots as an input parameter, do we need to require roots for this?
There was a problem hiding this comment.
Sure for now I can just remove this instead of making it not throw.
Longer term we might want to still enforce that we will only run these tools inside one of the specified roots, just to keep the AI on the rails. As an example, what if it somehow gets into the pub cache and starts trying to edit files or run commands 😱 .
This mixin will be used by anything that wants to look at what the current roots are, so we can share that logic across them.
Likely we will want to add this to various other tools (like the command line tools).