Awesome, thanks @akin909 for making the branch available! I wanted to try it out, will save me some time 💯

Ya, looks like it works pretty well! I noticed diagnostics are missing too unfortunately - I wonder if they'd be open for a PR to implement that? The rename and go-to definition issues might be on our end - need to check it out.

Also, FYI, if you set this:

    "language.typescript.rootFiles": ["tsconfig.json", "package.json"],

It shouldn't restart the process as frequently (so should perform better when switching files in the same workspace)

I think the best bet for us would be to see if we could help them implement the diagnostics, which is the main gap - and then integrate this in lieu of our in-proc strategy. From there, we could help implement the 'auto-import' functionality - looks like they have an issue tracking it already:
typescript-language-server/typescript-language-server#18

@bryphe I'm getting error markers and tss error and warning messages, or do you mean something different re diagnostics?
Thanks for the heads up re the root files setting will give that a go 👍

Codecov Report

Merging #1717 into master will decrease coverage by 8.06%.
The diff coverage is 25%.

@@            Coverage Diff             @@
##           master    #1717      +/-   ##
==========================================
- Coverage   45.43%   37.37%   -8.07%     
==========================================
  Files         361      297      -64     
  Lines       14563    12219    -2344     
  Branches     1912     1600     -312     
==========================================
- Hits         6617     4567    -2050     
+ Misses       7721     7402     -319     
- Partials      225      250      +25

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ae1224...70779bf. Read the comment docs.

Not sure the root files option actually makes things faster 😞 , also re diagnostics not sure all the issues with errors have gone away although the behaviour seems more realtime unless switching file when the lsp is restarting so old errors wait till its booted back up to clear

Not sure the root files option actually makes things faster 😞

Hmm, are you still seeing the server restart when switching files after setting that? Perhaps it's a different issue.

also re diagnostics not sure all the issues with errors have gone away although the behaviour seems more realtime unless switching file when the lsp is restarting so old errors wait till its booted back up to clear

Oh, cool! I'm glad it actually has diagnostics. I wasn't seeing them, but perhaps there is some pathing issue or something on Windows.. need to check further. I checked again in their code and it seems they are publishing diagnostics, so thats promising

If we can figure out that switching issue, seems like it's definitely worth bringing in! 👍 Then we can see if we can work with them to add the auto-import functionality

@bryphe re. the restarting of the lsp I initially looked at our client to see if it was triggering a restart however the flag shouldServerRestart we use doesn't seem to be malfunctioning but the lsp is restarting though I'm not clear as to why.

I've noted the following series of events

A didClose event is sent and the LSP responds by shutting down the lsp.

Fumbling in the dark here but I wonder if this might at all relate to our configuration in the lsp ?

Thanks for the investigation, @akin909 ! I'm stoked to integrate this and get out of the business of maintaining our own TypeScript layer 👍

A didClose event is sent and the LSP responds by shutting down the lsp.

The trace helps a lot. I think that the fault may be ours - we may be sending didClose too aggressively. According to the spec here: https://microsoft.github.io/language-server-protocol/overview we should be sending it when it's closed (when the editor is no longer tracking changes). However, we send it whenever we switch documents - so it might be that we need to change that behavior.

As a temporary troubleshooting measure, we could try commenting out this line:

Just to see how the language server behaves. If we find it's better after removing that, we might need to change the event (like, hook to an onBufferClose event versus the onBufferLeave event we listen to currently).

@bryphe thanks for the heads up was having difficulty figuring out what the significance of that sequence of events was, will give commenting that section out a shot

Edit: tried with the event changed to bufferClose which prevents the restarting however theres still a 2s approx lag on switching file. I'm wondering whether this might relate to the didOpen event as well as we send one on switching buffers whether or not the buffer was open but was hidden, although tbh it seems to be sporadic, the lag, as sometimes theres no delay and on others its more noticeable

Another issue which I think my be contributing to lagging or the server functionality cutting out is an error which seems to occur on trying to getDefinition

Seems to be referenced at microsoft/TypeScript#21818 but cant see any information that might help mitigate this 🤕

@bryphe started working on this again and I've managed to fix the rename bug, as well as the restarting bug - this required changing the logic we use to find a root dir as it was unfortunately not returning the correct directory causing the shouldServerRestart check to fail, as well as adding a onBufferDelete event to the editor to trigger the document close event.

The one outstanding issue I believe with this PR is lineCount error above with the textDocument/Definition it only occurs after a while of use and actually had a look in the tss.js which is the whole server (its 5mb) and I think it must be that we are sending some incorrect data at some point but I'm still not familiar enough to know how this works aka the definitions appear normally but at some point something problematic get sent to the tss would you have any ideas or suggestion re where to look?

Update on this I found: microsoft/TypeScript#16456 which seems to be a very similar (not the same issue) and the cause of it seems to be the language server slowly getting into a state of confusion because of problematic input/ casing issues. I'm thinking that somewhere along the way we post to the language server, details like line and column numbers perhaps some of this info is off (I know neovim sends line info as 0 based and perhaps it might relate to that?)

@bryphe started working on this again and I've managed to fix the rename bug, as well as the restarting bug - this required changing the logic we use to find a root dir as it was unfortunately not returning the correct directory causing the shouldServerRestart check to fail, as well as adding a onBufferDelete event to the editor to trigger the document close event.

Sounds like good progress! Thanks @akin909 .

I'm thinking that somewhere along the way we post to the language server, details like line and column numbers perhaps some of this info is off (I know neovim sends line info as 0 based and perhaps it might relate to that?)

Hmm, the sendRequest and sendNotification are the central places we send stuff here:

We log the request names, but you might want to log the args properties here.

What I'd really like is a way for us to test the LanguageClient independently of Oni there's really no dependency on the UI here. We could create a LanguageClient, and open some example TypeScript project / files programmatically (via appropriate sendRequest calls), and then validate that we get proper completions / definitions / etc. It might be a bit of work to get that working, but it'd give us a lot of confidence that our language layer is working correctly, independently of the UI - so it would make refactorings like #2235 safer, and let us exercise language support w/o necessarily needing a ton of CiTests.

Thanks for the heads up with those I will do some logging and try and figure out whats going on.

Re. testing the lsp independently that definitely seems v. useful/necessary, perhaps we can have another issue to outline how that might work? e.g. from what i've seen most lsps can be run from the command line so perhaps a process could be spawned and we use that to send various request and check the responses to ensure that things come and go as expected as you suggest. Although sounds like it might require a service as opposed to just spawning a process as a mock textDocument would be needed as well as content to send etc, or perhaps the service would require actually files to be created etc.

I think with this PR i'd like to focus on just getting theia-ide to work so that I can get back to the original goal which was to actually look at helping theia set up auto-import functionality once we know its compatible with oni and is what we are going to be/ can use.

Raised the issues regarding testing the lsp as #2246

Any idea when this is going to land?

@wesselvdv there've been quite a few errors crop up in trying to integrate this lsp, so has taken way longer than I initially hoped. There've have been quite a lot of very nice improvememts to that client since which would be awesome to integrate with oni. Tbh can't give an eta ill update the client and have another go with it to see if im still encountering the error which kills the lsp client (not sure what exactly is going on there i.e. is it an oni error or the client is erroring)