However, we don't want to break our Tern users so we fall back to Tern if TypeScript is not installed.

This could still be a breaking change if there's a user who prefers Tern for javascript and TSServer for typescript.

All around, this looks good to me, but I'll let @puremourning be the judge. One question though. If we are going to do this, considering that we don't usually have two servers for one language, shouldn't we have a plan for eventually dropping Tern?

Reviewed 54 of 55 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

This could still be a breaking change if there's a user who prefers Tern for javascript and TSServer for typescript.

That's true. Maybe we need an option after all. The issue with an option is that it still requires the user to restart the server which is inconvenient.

If we are going to do this, considering that we don't usually have two servers for one language, shouldn't we have a plan for eventually dropping Tern?

Put the Tern completer in low maintenance mode and drop it when it becomes irrelevant?

Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

You could say that people that want Tern for javascript and TSServer for typescript have a strong preference (for whatever reason). I'd expect those users to put a setting in their vimrc (or equivalent) and not change it unless they have to. For that reason, the option doesn't seem inconvenient.

drop it when it becomes irrelevant?

Sounds good. How will we know it became irrelevant?

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

Codecov Report

Merging #1036 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1036      +/-   ##
==========================================
+ Coverage   97.51%   97.53%   +0.01%     
==========================================
  Files          90       90              
  Lines        6958     6968      +10     
==========================================
+ Hits         6785     6796      +11     
+ Misses        173      172       -1

Reviewed 6 of 6 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

ycmd/tests/shutdown_test.py, line 93 at r2 (raw file):

                  'python',
                  'typescript' ]
                  'rust' ]

These two closing brackets are causing the tests to fail.

Comments from Reviewable

Added the use_tern_completer option to force the use of Tern for JavaScript even if TypeScript is installed. The option is disabled by default.

Reviewed 51 of 55 files at r1, 6 of 6 files at r2, 2 of 2 files at r3, 1 of 1 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

I still worry that this change is breaking by default. I have typescript installed and I build ycmd with --js-completer, but I use and have configuration for tern in my projects (i.e. a .tern-project). If I install this version of ycmd, my project stops working. I don't think we want that.

Perhaps we need to be more sympathetic with our compatibility? For example, if we detect a .tern-project (which we already do) and tern is set up, use the Tern completer, otherwise use the typescript completer ?

Review status: all files reviewed at latest revision, 1 unresolved discussion.

a discussion (no related file):
Does tsserver support things like node and jquery plugins/depencies etc. like tern does? If so do we know how you set them up? How do we set up things like dependency paths when using require js and other such? I assume this is all handled in the jsconfig.json ?

ycmd/completers/javascript/hook.py, line 32 at r5 (raw file):

def GetCompleter( user_options ):
  if ( not user_options[ 'use_tern_completer' ] and

I would rather avoid having an apology-option if we can avoid it. I think an alternative would be have ShouldEnableTernConpleter start returning False when .tern-project isn't found.

ycmd/tests/javascript/testdata/imports.js, line 1 at r5 (raw file):

import func from "library";

IIRC this is ECMAscript 5? How does one tell tsserver to use previous versions? Is there some typescript config file that we should document/link to etc.?

Comments from Reviewable

☔ The latest upstream changes (presumably #1047) made this pull request unmergeable. Please resolve the merge conflicts.

For example, if we detect a .tern-project (which we already do) and tern is set up, use the Tern completer, otherwise use the typescript completer ?

We can't detect if a .tern-project file exists because the request is not available when loading a completer. If we really want to be backward compatible, we first need to check if ycmd was built with --js-completer and use the Tern completer if that's the case; otherwise, we fall back to the TypeScript completer if TypeScript is installed. My concern with that approach is that if the user want to use the TypeScript completer for JavaScript but Tern has already been installed through the --js-completer option, he needs to manually remove the third_party/tern_runtime/node_modules folder. This is something that we'll need to document.

Review status: all files reviewed at latest revision, 4 unresolved discussions.

a discussion (no related file):

Does tsserver support things like node and jquery plugins/depencies etc. like tern does? If so do we know how you set them up?

Yes, TSServer does it by looking at the dependencies in package.json. The dependencies can also be set in the jsconfig.json file. See https://code.visualstudio.com/Docs/languages/javascript#_automatic-type-acquisition.

How do we set up things like dependency paths when using require js and other such?

AMD is not supported and will probably never be (since the technology is obsolete). That's IMO the only reason to keep Tern.

ycmd/completers/javascript/hook.py, line 32 at r5 (raw file):

I removed the option. We now attempt to load the Tern completer first then the TypeScript completer.

ycmd/tests/javascript/testdata/imports.js, line 1 at r5 (raw file):

IIRC this is ECMAscript 5?

No, it's ES2015 (ES 6).

How does one tell tsserver to use previous versions?

By specifying the target option in jsconfig.json (or tsconfig.json) e.g.

{
    "compilerOptions": {
        "target": "es5"
    }
}

Is there some typescript config file that we should document/link to etc.?

We could include these links:

• https://code.visualstudio.com/Docs/languages/javascript
• https://code.visualstudio.com/docs/languages/jsconfig
• http://json.schemastore.org/jsconfig

Comments from Reviewable

Right that makes sense.

Reviewed 9 of 13 files at r6, 2 of 2 files at r7.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

a discussion (no related file):

So tsserver assumes that you use npm ?

what's AMD ?

I love how things in javascript land become obsolete. Doesn't matter how many people are using them, someone on the internet declares that it is obsolete and that some equally crappy thing has taken its place.

Obsolete means no longer used or useful. Not that it is no longer recommended. slash rant.

Comments from Reviewable

Review status: all files reviewed at latest revision, 2 unresolved discussions.

a discussion (no related file):

So tsserver assumes that you use npm ?

Most likely since you need npm to install it but I can't say for sure.

what's AMD ?

AMD stands for Asynchronous Module Definition and is the API used by requirejs to load modules.

I love how things in javascript land become obsolete. Doesn't matter how many people are using them, someone on the internet declares that it is obsolete and that some equally crappy thing has taken its place.

Obsolete means no longer used or useful. Not that it is no longer recommended. slash rant.

I suppose "superseded" would be a better choice of word.

Comments from Reviewable

☔ The latest upstream changes (presumably #1028) made this pull request unmergeable. Please resolve the merge conflicts.

Normally I'd be very concerned with us breaking people by switching to a different backend. But since Tern is now unmaintained, IMO this concern should be much relaxed. Whoever is using Tern today will eventually have to migrate away from it because it's dead.

Thus, I think even a hard break where we stop supporting Tern completely might be acceptable. We could do this in stages where we deprecate Tern support and rip it out a few months later.

Thoughts?

Review status: 0 of 2 LGTMs obtained

Comments from Reviewable

As I said before, tern isn’t dead exactly: it still works and I still use it, and tsserver doesn’t support things in it that I need.

So in a survey of 1, 100% of Bens still need tern support.

If that means I have to maintain it in my fork, so be it, but I would be surprised if me and my colleagues are the only ones on that situation.

Review status: 0 of 2 LGTMs obtained

Comments from Reviewable

This is as far as I can see. Since I don't use javascript and don't have a great idea about tern vs tsserver, I'll let someone else call @zzbot.

Reviewed 3 of 13 files at r6, 1 of 2 files at r7, 1 of 5 files at r8, 1 of 1 files at r10.
Review status: 1 of 2 LGTMs obtained (and 1 stale)

Comments from Reviewable

☔ The latest upstream changes (presumably #1057) made this pull request unmergeable. Please resolve the merge conflicts.

@zzbot status

@zzbot r+

📌 Commit df75ef0 has been approved by micbou

⌛ Testing commit df75ef0 with merge e699495...

💔 Test failed - status-travis

@zzbot retry

⌛ Testing commit df75ef0 with merge 4a9e1d3...

💔 Test failed - status-travis

@zzbot r+

📌 Commit 8a02da4 has been approved by bstaletic

⌛ Testing commit 8a02da4 with merge c5ba63a...

☀️ Test successful - status-appveyor, status-travis
Approved by: bstaletic
Pushing c5ba63a to master...