Switch to Theia typescript language server #139

damieng · 2018-11-08T06:29:23Z

Fixes #127 and subsequently probably many others.

Thanks to @mattlyons0 for investigating the suitability of the Theia language server in conjunction with Atom's LSP support and IDE features.

mattlyons0 · 2018-11-08T06:53:06Z

package.json

@@ -50,7 +56,7 @@
  },
  "dependencies": {
    "atom-languageclient": "0.9.7",
-    "javascript-typescript-langserver": "^2.7.0"
+    "typescript-language-server": "0.3.6"


v0.3.6 doesn't have the hierarchical support fixes merged (the latest few 0.4.0-dev versions have it). Additionally none of these versions have the child_process forking implemented, its only implemented in PRs right now. Without the forking changes the shebang at the top of tsserver will be used which requires users have node installed locally and that PATH happens to resolve correctly (which it doesn't on osx using homebrew to install node).

I'd rather we move on without waiting for 0.4.0 for now as we actually already have a hierarchy anyway using the old api.

I did try using your fork but the yarn dependency was making things just plain not install for myself on Windows and another team member on macOS. I think we're better off either putting something in the README or some detection logic about node being required on the path until they merge the logic in upstream.

I think it might be best to deploy a built fork of the package to npm then with the forking logic in. Note some very interesting hacks I needed to get it working without: mattlyons0@f218f1d#diff-16b7d75b70953cbf4755508170f2f24cL34
The node binary wasn't resolving on startup (on OSX with node installed via homebrew, worked fine on Linux) which caused the plugin to crash, after ~10sec when startup had completed then I could disable and enable the plugin to restart everything and it would resolve. I put this hack in to get it to resolve on startup by having the user specify the folder which node binary was in.

I've built a version of typescript-language-server from https://github.com/mattlyons0/typescript-language-server/tree/release and published it to npm under @mattlyons/typescript-language-server. Feel free to give that a shot and grab the tarball if you wish to publish it under a package you own.

mattlyons0 · 2018-11-08T06:57:18Z

lib/main.js

+  getTypescriptServerPath() {
+    const tsServerPath = atom.config.get('ide-typescript.typescriptServerPath')
+    if (tsServerPath) return tsServerPath
+    return process.platform === 'win32' ? 'tsserver.cmd' : 'tsserver'


Since tsserver has a shebang at the top the electron built in version of node is not used with tsserver. None of this is merged in yet but a interesting detail of the PR most likely to be merged: typescript-language-server/typescript-language-server#88 is that it will only fork on files ending with .js, otherwise it will use child_process.exec on them (which will use the globally installed node version if it can correctly find it). Implementation wise I think this would mean to just use the lib/tsserver.js file that tsserver links to but for user configuration it might get interesting...

See: typescript-language-server/typescript-language-server#88 (comment)
Since this PR got merged in the config option for typescriptServerPath can be removed and it can always be `node_modules/typescript/lib/tsserver.js as long as the typescript package is a dependency.

Since a .js file is being invoked theia will start tsserver by forking the current node process (and will use the node version built into electron). Additionally I don't think there needs to be anything special done on windows anymore since hashbang isn't being relied on after this change (but haven't tested this).

By keeping this there though they can optionally use a different version of typescript from the one the Theia language server ships with? Is that even supported that end tho?

Yes it is, but it will only be opened "properly" (with forking logic) if the file ends with .js

What's interesting is since the bundled version of typescript is in the top level of the repository and they actually publish the /server folder, the published version of typescript-language-server on npm does not include the typescript package so it would need to be included by ide-typescript. See https://github.com/theia-ide/typescript-language-server/blob/master/server/package.json

Oh, I see, they don't include typescript with their language server so we have to include it and specify path to it. We could still let them override the path via a config setting but would need to change the default path to point to the one we bundle.

… runtime.

akosyakov · 2018-11-18T13:52:55Z

0.3.7 was released with typescript-language-server/typescript-language-server#88, please try

damieng · 2018-11-19T17:49:39Z

Thanks @akosyakov seems to be working great.

damieng · 2018-11-19T17:50:32Z

Hey @mattlyons0 can you give this PR a try/look over?

mattlyons0 · 2018-11-19T19:24:50Z

See https://github.com/atom/ide-typescript/pull/139/files#r234747489
Current behavior is basically it doesn't work out of the box for 2 reasons. Node needs to be installed and its path set in the configuration of this plugin and the typescript package needs to be installed. Both of these can be fixed with my comments.

mattlyons0 · 2018-11-19T20:34:25Z

Just confirmed 7dd7854 works on windows without node installed 🎉

mcous · 2018-11-20T00:21:30Z

lib/main.js

    return super.shouldStartForEditor(editor);
  }

+  validateTypeScriptServerPath() {
+    const tsServerPath = atom.config.get('ide-typescript.typeScriptServer.path')
+    if (fs.existsSync(tsServerPath)) return true


Hi! I'm not involved here or familiar with this code at all (sorry to just drop in like this), but I was playing around with this branch and noticed that on my machine this doesn't work because process.cwd() here is /. No matter what, I get the warning. It looks like this needs a path.join like shouldStartForEditor above.

But other than that this is working great for me! Thanks for your hard work!

No problem just dropping in :) I'll get that fixed before merging - it interestingly works here simply because I'm working on ide-typescript and so had launched atom from that folder :D

Most of the work was by @mattlyons0 so my hat is tipped to him.

mattlyons0 · 2018-11-28T02:26:50Z

Thanks for being so awesome and willing to look through this (rather large) change @damieng
Also thanks to @akosyakov for your work on Theia and being so receptive to PRs to support this effort. This is a huge improvement to Atom JS workflow and I hope to convert more people to Atom because of it

Aerijo · 2018-11-28T04:06:16Z

@damieng FYI the atom-languageclient readme refers to the old LS

damieng · 2018-11-28T04:20:09Z

Thanks for the reminder, I'll bump that once we publish the new version of ide-typescript.

atom/ide-typescript#139

Switch to Theia typescript language server

5b2cdb8

mattlyons0 reviewed Nov 8, 2018

View reviewed changes

damieng added 4 commits November 8, 2018 09:21

Code formatting supported in Theia language server.

5c3e704

Switch to MattLyon's fork until upstream supports specifying the node…

145f71d

… runtime.

Work around autocomplete issue in Theia caused by snippet property

3e1ed5e

⬆️ atom-languageclient

3ec3ca9

Switch from mattlyons fork of typescript-language-server to upstream

96a08b2

Bundle typescript and pass tsserver.js path to theia ls

7dd7854

Clarify TypeScript server path setting and check at runtime

c530022

mcous reviewed Nov 20, 2018

View reviewed changes

damieng added 3 commits November 19, 2018 17:42

Prefix unused variables with underscore to make tsc smile

8e63d22

Correct checking relative path of typescript server, clarify setting

aeba3f3

Merge branch 'master' into theia-language-server

24eae53

damieng merged commit 3ecece9 into master Nov 27, 2018

damieng deleted the theia-language-server branch November 27, 2018 17:40

yami-beta added a commit to yami-beta/dotfiles that referenced this pull request Dec 25, 2018

Use typescript-language-server

55d6cbe

atom/ide-typescript#139

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to Theia typescript language server #139

Switch to Theia typescript language server #139

damieng commented Nov 8, 2018

mattlyons0 Nov 8, 2018

damieng Nov 8, 2018 •

edited

Loading

mattlyons0 Nov 8, 2018 •

edited

Loading

mattlyons0 Nov 8, 2018

mattlyons0 Nov 19, 2018 •

edited

Loading

damieng Nov 19, 2018

mattlyons0 Nov 19, 2018

damieng Nov 19, 2018

mattlyons0 Nov 19, 2018

akosyakov commented Nov 18, 2018

damieng commented Nov 19, 2018

damieng commented Nov 19, 2018

mattlyons0 commented Nov 19, 2018

mattlyons0 commented Nov 19, 2018

mcous Nov 20, 2018

damieng Nov 20, 2018

mattlyons0 commented Nov 28, 2018

Aerijo commented Nov 28, 2018

damieng commented Nov 28, 2018

Switch to Theia typescript language server #139

Switch to Theia typescript language server #139

Conversation

damieng commented Nov 8, 2018

Choose a reason for hiding this comment

damieng Nov 8, 2018 • edited Loading

Choose a reason for hiding this comment

mattlyons0 Nov 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattlyons0 Nov 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov commented Nov 18, 2018

damieng commented Nov 19, 2018

damieng commented Nov 19, 2018

mattlyons0 commented Nov 19, 2018

mattlyons0 commented Nov 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattlyons0 commented Nov 28, 2018

Aerijo commented Nov 28, 2018

damieng commented Nov 28, 2018

damieng Nov 8, 2018 •

edited

Loading

mattlyons0 Nov 8, 2018 •

edited

Loading

mattlyons0 Nov 19, 2018 •

edited

Loading