Repository navigation
Change InteractiveClient to instead by an LSP client feature - #6078
Conversation
|
@h9jiang what are your thoughts on this? Note: I don't have automated tests for this yet, but I'd like to add some so that changes like this don't rely on manual testing. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the interactive forms handling by replacing the InteractiveLanguageClient subclass with a standalone InteractiveFormsFeature class that wraps a standard LanguageClient. The review comments identify a critical TypeScript compilation issue where the protected method handleFailedRequest is accessed directly on the LanguageClient instance, and suggest casting the client to any to resolve the error.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## interactive-forms #6078 +/- ##
=====================================================
- Coverage 70.11% 70.03% -0.09%
=====================================================
Files 173 173
Lines 13234 13265 +31
Branches 2661 2665 +4
=====================================================
+ Hits 9279 9290 +11
- Misses 3456 3478 +22
+ Partials 499 497 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
48d42d5 to
18d3efb
Compare
|
Hi Danny, I tried to pull the changes from form.ts from dart-code back to vscode-go, other than the issue mentioned previously "the method name change from dispose() -> clear()", I do not observe any compatibility issue. (I wish vscode api have compatibility promise...). This also means, it is time for me to bump the vscode api version. :D I will take a deeper look later this week, and left my comments before the next Monday. If you have any deadline for this, could you let me know so I prioritize reviewing this before others. I'm still busy with the extension release and go release. Thank you so much for putting this together! |
I think the difference is in the VS Code LSP client, not VS Code API itself? If the changes are only in the pre-release version, you could stick with the current stable one (and renamed the methods), and Dart-Code can be the "forked" version for now until it ships. Once things have settled down a bit, perhaps we should also create a repo to store a source-of-truth version of this code (and perhaps add some automated tests)?
No real deadline. It would be nice if it was merged in before the next release (around the end of the month) to make it easier for others to test, but it will still be behind a flag for that release at least. If you don't get chance to review things, I'm also not against merging code in we want to ship, and then addressing any feedback later (again, we can consider this a "forked" version in the short-term, and then align on the exact code after). Thanks! |
| const middleware = interactiveOptions.middleware; | ||
| export class InteractiveFormsFeature { | ||
| constructor(private readonly client: LanguageClient) { | ||
| this.addMiddleware(); |
There was a problem hiding this comment.
This may seems to be a naive question, but maybe there is further optimization we can do here. I'm still new to the static feature registration mechanism, please correct me if I'm wrong.
Is it possible to put this function call this.addMiddleware() to initialize() method? IIUC, the initialize() is called when the language server returns the server capabilities.
So in initialize(), we could already check if the server support interactive refactoring, then we can call addMiddleware conditionally based on whether the language server have the support for interactivity.
But this can only be done if the language client promises to wait for all the feature's (static / dynamic) initialize() to finish before executing any LSP methods. (Basically add the middleware after the initialize request is done but before any other LSP request is sent)
Right now, I think, the implementation is always add the middleware and we check the "server capability" in every execute command request.
Let me know what you think.
There was a problem hiding this comment.
Good question! It turns out we can't do this in initialize because the built-in ExecuteCommandFeature captures the middleware inside initialize() (via register()) and therefore we install it too late.
However, there is a preInitialize() that fires for all features first, and it does work there. It does rely on ExecuteCommandFeature never switching to capture middleware during preInitialize though.
I've pushed this change - please take a look. I didn't duplicate checking the kinds of support for installing middleware though, only that the object exists at all. Seem reasonable?
There was a problem hiding this comment.
Hi Danny, I think after taking another look, I think it is not the ideal. Sorry for giving a bad suggestion.
It turns out there two kinds of restart,
- restart by creating a new language client and a new language server process
- restart by only create new language server process (can be done by errorHandler return
CloseAction.Restart)
Consider the addMiddleware function a "swap" function that replace the existing execute command middleware with our version... It works fine in first scenario but not the second one.
In the first scenario, the langauge client will be disposed, so it is a fresh client and the middleware.executeCommand is the user provided one. However, in the second case, the client is not disposed, so the middleware.executeCommand is already swapped by us. Then we "swap" again. To fix this,
- use the original approach, manually check server capability on executeCommand
- have some memorized boolean, indicating whether the executeCommand has been swapped. If is already swapped, stop swap it. If the server originally support it, now no longer, we have to "parse" it again.
So my suggestion would be revert to the original state. I'm sincerely sorry for the confusion caused by me.
| export class LspAnalyzer extends Analyzer { | ||
| public readonly client: LanguageClient; | ||
| public readonly fileTracker: FileTracker; | ||
| private readonly interactiveClient: InteractiveFormsFeature; |
There was a problem hiding this comment.
Just a curious question, is making the feature private property necessary or it is because following the practices.
There was a problem hiding this comment.
Generally I prefer to keep things private if not needed (to avoid accidentally depending on something that wasn't intended). The others are public only because they are exposed as part of the private API used by test runs.
There was a problem hiding this comment.
Oh, maybe I misunderstood the question. If the question was "why is it private when others are public", the comment above is correct.
If the question is why do we have the field at all, then the answer is, I don't know - I did previously start writing tests (but aborted because I wanted to lock down the API first), so maybe I was exposing it for that. So, I have removed it.
And a related question, is "why do we have a .feature" field in InteractiveFormsFeature and I also think there's no good reason (I copied this pattern from a previous feature, where I had adapted a previous Provider into a feature). So I've also removed that.
So now the registration code is simply:
this.client.registerFeature(new InteractiveFormsFeature(this.client));
No field in the analyzer class, and no .feature on the end.
There was a problem hiding this comment.
Sounds good, if we need to write test for it, I'm also agree we can make it public for convenience.
There was a problem hiding this comment.
Hi Danny, I think the idea of migrating from subclass to static feature make sense. This avoid the inherit problem as you mentioned. I left a few comments in the PR. Please take a look.
Over all the PR looks good to me. The only important comment is whether we can make the addMiddleware to initialize() method.
But just to confirm a developer user journal will looks like below:
- Copy the
form.ts - Write the middleware like below
export class MyExtensionAnalyzer {
public readonly client: LanguageClient;
private readonly interactiveClient: InteractiveFormsFeature;
constructor(serverOptions: ServerOptions) {
// 2. Type your middleware as InteractiveMiddleware
const middleware: InteractiveMiddleware = {
provideHover: (document, position, token, next) => next(document, position, token),
};
const clientOptions: InteractiveLanguageClientOptions = {
documentSelector: [{ scheme: 'file', language: 'mydsl' }],
middleware,
};
this.client = new LanguageClient("myDSLClient", "My DSL Server", serverOptions, clientOptions);
// 3. Set up and register the interactive feature
this.interactiveClient = new InteractiveFormsFeature(this.client);
this.client.registerFeature(this.interactiveClient.feature);
void this.client.start();
}
}
My understanding is, the extension author can either create the client using middleware or the InteractiveMiddleware, either one of them is accepted. By creating a regular middleware means the extension author does not need any interactive refactoring customization. (just calling next()). If the extension author create a InteractiveMiddleware, the extension author can customize lsp (relevant to the interactive refactoring). Am I understanding the user journey correctly?
Yes, you are correct. Let me bump the LSP client version in vscode-go.
For adoption and releasing, this is what I'm thinking. For vscode-go, we already released v0.54.0, using the "sublass" solution. At the end of the day, no one can import "form.ts" directly from vscode-go and we have never announced this publicly ever. So consider the current implementation using "subclass" solution a prototype implementation for the proposal that can be deprecated anytime. I will update vscode-go to use "static feature" solution once your CL is merged. When we actually announce it, we will have another vscode-go release (e.g. v0.54.1 or v0.56.0) that is using "static feature" solution. So when we announce this, we can make sure both the dart-code and vscode-go extension use the same "static feature" solution.
I think this would make the most sense to have it prepared somewhere serving like a library. (but we will need to maintain it if we ever publish it as a package / module). Companies does have a little bit restrictive rule regarding in term of "releasing" especially it is released under an "organization". So I'm also ok with just asking the extension author to copy the |
Yep, that's correct. Although I made some tweaks to simplify the feature registration a little (removing
I don't recall if the required changes are in a stable version or only the last pre-release. If only the pre-release and you want to stick with the stable version for now, we could make a central version against that, and just have a slightly-tweaked version in Dart-Code until the LSP client library ships stable. I don't know what your appetite is for using a pre-release LSP client.
That sounds fine, although I don't think it has to be a requirement that we're on exactly the same code immediately, since as you note it's just internal code nobody else is referencing. If we need to iterate on this a bit more, it needn't hold up your announcement of the feature?
Yeah, I think that's also fine (particularly while we're still tweaking things and shipping it ourselves). Although, it might still be good to have a single central place for |
|
There shouldn't be any complications related to hosting it in the |
|
Hi @DanTup, thanks for preparing the CL, I think the CL looks good to me. The only comment I have is regarding the Other than that, this looks good to me. (I don't think I can cast a vote though), consider it +2 other than the one comment above. (I can sync vscode-go once you merge this PR or if you want to become a contributor to vscode-go :D) For publishing code, I think we can first share the code as is (asking extension author to copy and paste them) while waiting for the feedback from LSP. If LSP expressed interest to adopt the solution, we don't need to publish it anymore. If there is very slow (or no) progress on LSP, we can publish and maintain that. (There is some internal process we need to publish if we want to publish to an org under Google). |
cf02498 to
4bed1ec
Compare
3c83172 to
28f3455
Compare
This makes it easier to conditionally register, and doesn't interfere if a client already has its own hierarchy.
…rmsFeature the feature itself
c2ba60c to
8aa16bf
Compare
8aa16bf to
e6657d4
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the interactive forms feature by converting the InteractiveLanguageClient subclass into a standalone StaticFeature named InteractiveFormsFeature, which is conditionally registered based on a new experimentalInteractiveForms configuration. The review feedback recommends wrapping the new conditional block in curly braces for consistency and removing several redundant type assertions in form.ts to clean up the code.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
I think you should be able to review it. It currently says "@h9jiang I have moved the middleware back to the constructor and addressed some other minor issues (AI review feedback, and resolving some additional lints that fired when rebasing, since I'd upgraded some deps). If this still looks good to you, I will merge it into the There is also a PR open at #6077 with some questions around handling of optional values that I'd like your input on (which may be worth resolving before you take a copy of I will also open an issue to discuss validation shortly, because I think having some kind of client-side validation would make the experience much nicer (the problem with |
Thanks. I replied last week. But the problem is, I don't know how to use github review... I write my comment and click Sorry for that. |
|
Thanks!
Ah, lol. No worries. You are definitely not alone in this (although, I sometimes I do the same in Gerrit too and only notice when it shows up in "Has draft comments" at the top of the dashboard 😆). I'll merge this one and I'll review your comments on the other one properly tomorrow - thanks! |
3 participants
This makes it easier to conditionally register, and doesn't interfere if a client already has its own hierarchy.