TypeScript: Hook up auto-import functionality

[bryphe]

Splitting out from #1326 - integrate 'auto-import' functionality from TypeScript. This should generalize to other completion providers, too.

• Implement additionalTextEdits and textEdit for CompletionItems as defined by the completion request in the language server protocol: https://microsoft.github.io/language-server-protocol/specification
• Update our TypeScriptServerHost to send additionalTextEdits for auto-imports. This will involve settting the includeExternalModuleExports for the completions request, hooking up entry details resolution to get information about the additional code actions as part of the details request, and resolving those code actions to text edits. Looks like we'll need to plumb the 'source' through somehow, as this is needed when calling to get details.

[akinsho]

@bryphe re. the additionalTextEdits can I clarify, from the docs you linked it seems as though there ought to be text edits returned as part of the completion items as the interface includes it as an optional key, as far as you are are should these be returned sometimes without having to change the args sent to the lsp (I've briefly logged the response and never seem to get any textEdit fields back in the completion responses).

[bryphe]

can I clarify, from the docs you linked it seems as though there ought to be text edits returned as part of the completion items as the interface includes it as an optional key, as far as you are are should these be returned sometimes without having to change the args sent to the lsp

Sure thing - I don't believe there are any additional capabilities we need to specify (looking at the language server specification, specifically the completion capabilities in the TextDopcumentClietnCapabilities).

(I've briefly logged the response and never seem to get any textEdit fields back in the completion responses).

Hmm, which language server were you testing it out with? For typescript / javascript, it's expected, since we haven't wired up the ability to pass up the additionalTextEdits yet. So we'd have to check with the language server you're using, if it supports it.

[akinsho]

@bryphe I hadn't quite got the flow correct in my head I found that where I was looking was downstream/ or upstream 🤔 in the LS but in the actual typescript client I found that the result is being trimmed so going to have a closer look at that, I'm just investigating a little bit for now as there are a lot of unknowns here for me as I've not seen much of this part of the code base.

[bryphe]

Cool, would be great to have help here! 👍 Let me know if you have any questions.

The typescript standalone server actually implements its own protocol here: https://github.com/Microsoft/TypeScript/wiki/Standalone-Server-%28tsserver%29

So what we have is basically a 'pseudo' language server implemented in 'oni-plugin-typescript' here: https://github.com/onivim/oni/blob/bff79031676dc6fbcbbfbf7887b8df2bf86c908e/vim/core/oni-plugin-typescript/src/index.ts#L62 That acts to translate the tsserver protocol into the language server protocol. Ideally, at some point, we could switch to a real LSP implementation - but none of the ones I see do incremental document sync which is important for performance.

So for this work item, we'd need to implement both sides - translate the tsserver completion info into the additionalTextEdits that the language server protocol expects, and then on the language client side in Oni, handle that when we commit a completion.

[akinsho]

@bryphe given I raised it and probably the most motivated person to see this implemented I'll assign myself if that's okay but will definitely need to pick your brain at some point no doubt re the lsp and tss

[bryphe]

@Akin909 , that's awesome, I really appreciate the help! There are a bunch of improvements / issues across this layer, so having you know how it works too would be amazing 👍

but will definitely need to pick your brain at some point no doubt re the lsp and tss

Sure thing, keep me posted

[akinsho]

@bryphe having just looked at another lsp here (to borrow ideas re how they implement this, I was taken aback by how enormous a task for oni it is / could be to replicate this functionality natively, can I ask a bit more about the functionality its missing re incremental document sync and how the available lsps currently implement it?

[bryphe]

Here's some info on the TextDocumentSyncKind here: https://github.com/Microsoft/language-server-protocol/blob/gh-pages/specification.md#initialize-request-leftwards_arrow_with_hook

(Search for Incremental). It's important for performance that our language servers support this, otherwise it has to re-parse the file for every change, even just a single character. For TypeScript, this would be really painful when I worked on large files (>1000 lines).

I'm open to switching to another LSP (I would actually prefer to use an established LSP as opposed to maintaining or own implementation!) - as long as we aren't regressing any functionality.

Unfortunately the sourcegraph LSP doesn't implement this: https://github.com/sourcegraph/javascript-typescript-langserver/blob/2cac6cb67f906e7af7a1a7c850b8145a0ae6eb55/src/typescript-service.ts#L294

In addition, doesn't seem like they implement the auto-import functionality anyway - that would happen in the getCompletion/getCompletionItemResolver functionality: https://github.com/sourcegraph/javascript-typescript-langserver/blob/2cac6cb67f906e7af7a1a7c850b8145a0ae6eb55/src/typescript-service.ts#L1223 So it really wouldn't help us anyway, today 😦

[bryphe]

FYI, I saw this other language server implementation for TypeScript that does support incremental updates: https://github.com/theia-ide/typescript-language-server

(Doesn't look like it has 'auto-import' functionality, though). Might be worth looking to see if we can switch to that implementation?

[akinsho]

@bryphe just gave it a go locally and seems really nice, (which is to say can't really tell the difference, still seems fast etc.) although rename seems to implode aka trying to rename threw an error cannot read property start of undefined pointing towards the range.start although tbh I havent been able to get rename to work consistently so not sure if that's on Oni's end.

Another thing I noticed which isn't really a bug is that the LSP reports an error if there is no content for a definition which doesn't seems like an error thankfully it doesn't raise a notification since its soo frequent but I wonder if it ought to be interpreted as an error.

Regarding auto-import I think I assumed it was a codeAction from the lsp but I cant really find many examples of a standard way of implementing it. nvim-typescript and tsuquyomi both vim plugins seems to use a mixture of lsp features as well as adhoc logic on top and I have no idea where in vscode it's implemented so not sure if its functionality that's been pieced together based on some of what the lsp returns or just a suggested code action in which case if theia-ide supports code actions then its a matter of implementing a subsection of them

[bryphe]

@bryphe just gave it a go locally and seems really nice, (which is to say can't really tell the difference, still seems fast etc.) although rename seems to implode aka trying to rename threw an error cannot read property start of undefined pointing towards the range.start although tbh I havent been able to get rename to work consistently so not sure if that's on Oni's end.

Nice! 👍 It's possible it's even faster than our current strategy, because it'd be running asynchronously as a separate process (our current typescript implementation is in-process, which is why things like workspace symbols block). I'd be open to switching to this - it'd be nice not to maintain our own strategy, and there are lots of bugs we have in our current implementation like #1530 #1344 #1588

The rename could be an issue on our end too.

Regarding auto-import I think I assumed it was a codeAction from the lsp but I cant really find many examples of a standard way of implementing it.

It's a bit of both - it comes through as an additional code action as part of the completion item. It doesn't look like it's implemented in the theia-ide's language server, but it would be a great place to add it. It looks like it supports code actions in general, just not the auto-import ones on adding a completion item yet.

[akinsho]

@bryphe I think you might be right about it being faster having used it for a bit longer now although there's an issue when switching buffers which I think is that the lsp restarts on every buffer switch which I think is probably a way of checking the right lsp is open for the right filetype not sure how this check is done but think a check to see if the filetype changes before the lsp restarts would prevent this.

Another thing I noted is that though the symbol search is faster it still blocks looking at how we populate the menu, it seems that we use the very cool observable function which takes the latest value after 25ms not sure specifically if its this but seems when it finally takes a value the ui then blocks till the async call to the lsp is complete.

I think not knowing what RxJs provides re this functionality it might be better there to have a function which waited a bit (I think 25ms is too short) as logging the functionality there its far too quick to allow several characters to actually be typed so trying to spell app hitting a starts a search ap a new search and app another which might have been the desired result but my thinking was that likely the intent is to allow the user to get the desired search term or part of it out.

I'm not sure whats possible but if the search waited for say 50-100ms then executed a search in a non-blocking fashion then re-executed the same functionality so another 50-100 was awaited then re-ran then next search this would solve the blocking issue