Make OnAutoInsert more reliable and add integration tests #7230
Conversation
| // We explicitly register against the server's didChange notification (instead of the VSCode workspace API) | ||
| // as we want to ensure that the server has processed the change before we request edits from auto insert. | ||
| // The VSCode workspace API will sometimes call this before the LSP client can send the change, leading to auto insert not working. | ||
| languageClient.getFeature('textDocument/didChange').onNotificationSent(async (event) => { |
There was a problem hiding this comment.
I discovered this when I was writing the integration tests (and I have had very occasional reports of it not working in the wild).
Essentially, it was possible for our handler of vscode.workspace.onDidChangeTextDocument to make a request to the LSP server before the LSP client handled the change notification and made a didChange request.
This resulted in an autoinsert being requested before the server knew about the text change (verified this ordering in the logs).
I modified this to instead get triggered after the client sends the didChange event to the server, ensuring the server gets that first. I also had to update the implementation as now all the types were LSP types and not vscode types.
There was a problem hiding this comment.
Also confirmed on the vscode side that languageClient.getFeature('textDocument/didChange').onNotificationSent is the right way to go.
Adds an integration test and fixes a race I discovered while writing the test.