[Discussion] Would you be willing to consider a different Language Server?

[mattlyons0]

Has it ever been considered to switch away from SourceGraph?
I was unable to use this plugin due to having symlink cycles (because I'm using lerna) and my understanding is that the SourceGraph team does not want to support this use case. I feel like in a lot of projects this leaves the plugin completely broken and infinitely using CPU until the process runs out of memory.

As a proof of concept I forked this repo and used Theia Language Server which at the moment seems to be a bit better maintained. My proof of concept is up at https://github.com/mattlyons0/ide-typescript-theia (and does have a few rough edges, on my Mac the tsserver is failing to find node on startup (which can be solved by toggling the plugin disabled then enabled after startup) but its working fine on my Linux box). (fixed in typescript-language-server/typescript-language-server#88 ) It seems to have better performance, no more infinite loops and a bit better feature support than SourceGraph (formatting hook is supported). The only downside I found is that the outline is no longer a tree but all on the same level, I'm not quite sure why this is (or how the tree is generated) but the responses to textDocument/documentSymbol after a brief investigation seemed to be identical. This has been fixed upstream: typescript-language-server/typescript-language-server#69

In summary I'm wondering if you have considered switching to a different Language Server, especially given how easy it is and the issues that have been cropping up with SourceGraph: #70 #95 #113 to name a few... Also I've noticed that hyperclicking into definition files actually works and doesn't open a broken URL which is a plus. All in all I feel the Theia Language Server has been rock solid and solves many issues with the current SourceGraph Server.

[damieng]

We would definitely be open to considering another language server - the project is about getting a good TypeScript experience for Atom IDE and we're not fixed on any specific implementation.

I'll try and see if I can find some time to check out what you've worked on, thanks for the heads-up and I'll let you know if/how we can proceed.

[akosyakov]

The only downside I found is that the outline is no longer a tree but all on the same level, I'm not quite sure why this is (or how the tree is generated) but the responses to textDocument/documentSymbol after a brief investigation seemed to be identical.

@mattlyons0 We've implemented the hierarchical document symbols recently, could be the support for non-hierarchical symbols broke. Could you open an issue against https://github.com/theia-ide/typescript-language-server/ with the repo which you've tried? We'll have a look.

If you have any other issues please file them. We are looking for opportunities to improve the server :)

[damieng]

The current atom-ide uses container names as well as ranges to figure out a tree based view using the existing outline api. It works pretty nicely although some servers fail to do it as VSCode makes no such attempt to structure outline in a tree.

[mattlyons0]

Thats it, container names are missing from the Theia textDocument/documentSymbol responses, tracking here: typescript-language-server/typescript-language-server#68

[akosyakov]

@mattlyons0 I've published a fix as a dev version: typescript-language-server@0.4.0-dev.a1b151f. 0.3.4 was published with a fix. Could you try it? typescript-language-server/typescript-language-server#69

The current atom-ide uses container names as well as ranges to figure out a tree based view using the existing outline api. It works pretty nicely although some servers fail to do it as VSCode makes no such attempt to structure outline in a tree.

Yes, we do something similar in Theia for servers not supporting hierarchical symbols. If a server supports then it is easier: a server provides already hierarchically structured symbols and a client only need to render them. Theia TS server supports it: https://github.com/theia-ide/typescript-language-server/blob/2cd55b3280d329fef53a41032a319940524889ae/server/src/document-symbol.ts#L34

[mattlyons0]

I updated the deps in my fork. It works perfectly, thanks for the quick response!

[k-ode]

Just tested this and it fixed the current version not respecting my tsconfig.json (I'm guessing it's the same issue as this #115). Diagnostics in the gutter seems to have disappeared though, but that might be expected/unrelated. They do show up in the diagnostics table.

[akosyakov]

@kimgronqvist how can I try it? sorry, I am not an expert in Atom packages. I would like to reproduce your issue with missing diagnostics in the gutter. We have it in Gitpod, as an alternative, you could try to reproduce it there for your project. It will be easier for me to track it down then.

[mattlyons0]

@kimgronqvist I cannot reproduce that, diagnostics are showing in both the gutter and the table for me. I have noticed however they are a little buggy when switching between plugins (for example if I disable ide-typescript when I had diagnostics and enable ide-typescript-theia the diagnostics will actually duplicate). This and the issue you are having must be a upstream issue in either atom-ide or atom-languageclient as all we do is pas the diagnostics messages to the upstream packages, if the table is working the messages must have been sent correctly. Maybe reloading atom would fix it? Window: Reload?

[damieng]

The ide diagnostics are a push model and we do not de-duplicate between extensions (and support multiple extensions at the same time). Disabling an extension can leave its diagnostics around until you restart Atom.

[k-ode]

Seems to be a typical Windows-path related issue :)

The problem is that diagnostics never get associated with the correct file. The path I get from the language server looks like this: c:\typescript-project\test.ts. In atom-ide-ui, the path that this is matched against is C:\typescript-project\test.ts.

So the solution would either be for the language server to make sure the drive letter is capitalized for Windows, or for atom-ide-ui to ignore path case on Windows.

[akosyakov]

@kimgronqvist @mattlyons0 I've published a new dev version typescript-language-server@0.4.0-dev.3a9d7e7 with typescript-language-server/typescript-language-server#75. Can you check whether it makes any difference?

[k-ode]

@akosyakov Works great!

[akosyakov]

Thanks for checking, 0.3.6 is published with a fix!

(and does have a few rough edges, on my Mac the tsserver is failing to find node on startup (which can be solved by toggling the plugin disabled then enabled after startup) but its working fine on my Linux box)

@mattlyons0 is it because on mac you don't have globally installed typescript? The server tries to look up typescript first in a user project, then in global modules, and then one which is bundled. For Gitpod and Theia, we bundle typescript that there is always one to fall back. Maybe you should try the same.

[mattlyons0]

@akosyakov I actually already have the package installing typescript, but the issue seems to be related to the way Electron handles child processes and lack of setting them up correctly upon startup. See mattlyons0#1 It looks like the typescript server was failing to find nodejs. It seemed that the hook which runs to enable the plugin upon Atom startup (or window reload) wasn't passing the environment in correctly. The seemingly weird fix I made was to set the environment only to that of nodejs mattlyons0@713eae1#diff-16b7d75b70953cbf4755508170f2f24cR33

[damieng]

If you want to run a specific node binary for your language server then you should not call super.spawnChildNode at all - that functions job is to spawn a child using the version of node built in to atom (which is actually the atom binary itself) rather than a standalone node binary installed on the users system.

[mattlyons0]

Hm so all I'm doing is passing the file to run in node https://github.com/mattlyons0/ide-typescript-theia/blob/master/lib/main.js#L32 but it has a hashbang at the top https://github.com/theia-ide/typescript-language-server/blob/master/server/src/cli.ts#L1 which is being executed since super.spawnChildNode just uses child_process's spawn.

I don't understand how this is different than what you are doing though, since the file you call also has the same hashbang https://github.com/sourcegraph/javascript-typescript-langserver/blob/master/src/language-server-stdio.ts#L1

My intuition when I was debugging the issue was that something within theia like the typescript package was trying to start node itself and failing to do so.

[akosyakov]

My intuition when I was debugging the issue was that something within theia like the typescript package was trying to start node itself and failing to do so.

The language server is a wrapper around tsserver. tsserver is as a separate node process: https://github.com/theia-ide/typescript-language-server/blob/2a13b53eaebaa962eb9e94a163e8b617a18569fd/server/src/tsp-client.ts#L105

[mattlyons0]

Thats super helpful to know. Okay I've looked into the issue deeper and fully understand how all the process spawning works. There is a way to get Theia running using the built in node version (built in to Atom) but it will require some upstream changes. I will discuss in the Theia Repo

[damieng]

Is there a working PR for this I could take a look at?

[k-ode]

I can prepare one if @mattlyons0 can't. Been using his fork for some time now without problems.

[mattlyons0]

There is this: https://github.com/mattlyons0/ide-typescript-theia
Clone it, run yarn install then apm link it (also you need to toggle it off then on to enable it since apm link doesn't initialize the package).

I use it daily at work and ever since I fixed using the global version of node vs the atom electron version I haven't had any problems.

Notice it has a dependency on my fork of the upstream typescript-language-server which is to support using child_process forking behavior instead of spawning (for creating a node sub process). Since its pointing to a git repo not a built release yarn must be used to install. I have a PR open for merging this in and there is another (more conservative but arguably better implementation) open as well: typescript-language-server/typescript-language-server#88

[saadq]

Woah, ide-typescript-theia is working awesomely! It starts working instantly and I'm no longer getting issues about modules (#43) that I did with ide-typescript.