tide spawns extraneous tsserver instances when navigating to files in node_modules

[lddubeau]

Checklist

• I have searched both open and closed issues and cannot find a duplicate.
• I can reproduce the problem with the latest version of the relevant packages.
• The problem still occurs after I issued M-x tide-restart-server in the buffer where I had the problem.

The rest of the checklist does not apply to the situation being reported.

Relevant Version Numbers

• Tide: commit 7c5ee30
• TypeScript: 2.7-2.9
• Emacs: 25.1.1

Steps to Reproduce the Bug

It is assumed your Emacs automatically starts tide.

1. Clone https://github.com/lddubeau/tide-spurious-tsservers.git

2. Open src/main.ts.

3. Move the pointer to Observable on the first line.

4. Hit M-..

Expected Behavior

One tsserver is started for src/main.ts, which uses src/tsconfig.json. When navigating to the file that defines Observable, a new tsserver should not be started. tide should instead use the tsserver started for src/main.ts.

Actual Behavior

Two tsserver instances are launched.

Observations

Besides possibly causing differences in error reporting between running tsc and what a user sees in Emacs, this also causes tide to spawn a bunch of unnecessary instances of tsserver.

The 2nd file that is opened when navigating to the definition of Observable is already part of the project that covers src/main.ts (otherwise tide would not be able to find the definition of Observable). Tide would have to pass that information along so that the buffer opened for the 2nd file does not try to figure out which project pertains to it.

[ananthakumaran]

Can you share some errors that are different between tsc and tsserver. Fixing this might not be straightforward as we are now saying find-file and M-. will do different things.

[lddubeau]

With the example repo I gave in the initial report, I get this with tsc:

$ tsc -p src/tsconfig.json 
node_modules/rxjs/internal/Observable.d.ts(82,59): error TS2693: 'Promise' only refers to a type, but is being used as a value here.

But when I navigate to Observable.d.ts through M-., I get a bunch of Cannot find name: Promise. And the error that tsc reported is not present.

I can tweak my tsc command or my tsconfig.json to get rid of the error when tsc runs, but there is nothing I can do to get rid of the errors reported by tide in the buffer for Observable.d.ts. If I want to go check what Promise is, I'm out of luck because tsserver does not know what Promise is.

I'm more concerned by the number of tsserver instances launched. The cases where there's a divergence in error reporting are not frequent, and those cases that would occur can probably just be mentally ignored. The extra tsserver instances launched, however, consume extra resources, and there's no amount of ignoring them that will make them consume less resources.

Fixing this might not be straightforward as we are now saying find-file and M-. will do different things.

True.

[josteink]

Fixing this might not be straightforward as we are now saying find-file and M-. will do different things.

True.

So how about ... making that not true then?

Let's say tide in general (and thus also for find-file) shouldn't automatically/always assume that a file in a nested node_modules folder represents a new project-root, even when it has a project.json or a tsconfig.json.

I know there are systems where the current behaviour is desired (i.e. like LERNA), but I don't use LERNA and I don't think everyone else is doing that either, so optimizing for that use-case only seems like an odd decision.

We could at least make in an option to search further up the path when the current project-path is resolved to something which is a subfolder of "node_modules". As far as I can see, that would kill this issue in one simple sweep.

And maybe only do that when the resolved folder doesn't contain a .git subfolder, making the .git-folder a marker of sorts that this something you actually work on independently?

These are just a few ways we can go at this which all:

1. gives consistent behaviour across find-file and M-.
2. does not complicate our codebase noticeably (one deterministic functioned changed/affected only)
3. should keep all our users happy.

If none of these are optimal, I'm sure there are other reasonably simple ways too.

What do you guys think?

[ananthakumaran]

We could at least make in an option to search further up the path when the current project-path is resolved to something which is a subfolder of "node_modules". As far as I can see, that would kill this issue in one simple sweep.

There might not be a parent-child relation in the folder structure always, for example we ship the typings for standard library inside tsserver folder. npm link is another example.

#256 might fix this issue if properly solved. Then we will have this problem only if we have to start multiple version of tsserver.

[lddubeau]

@josteink

I know there are systems where the current behaviour is desired (i.e. like LERNA), but I don't use LERNA and I don't think everyone else is doing that either, so optimizing for that use-case only seems like an odd decision.

typedoc (which I contribute to) is planning to use Lerna. I also have projects that I'm thinking of moving to Lerna.

We could at least make in an option to search further up the path when the current project-path is resolved to something which is a subfolder of "node_modules". As far as I can see, that would kill this issue in one simple sweep.

That would not solve the issue I reported. I think you misunderstood the details of the problem. The issue is not that tide finds a tsconfig.json under node_modules that confuses it. The issue is that there is no tsconfig.json to be found. In the example repo I created, there is no tsconfig.json in the root of the repo. tide searches up the path all the way to the root of the file system and does not find a tsconfig.json file. The files in node_modules/rxjs are therefore considered to be outside of any project.

I've not yet run into a library that bundles a tsconfig.json with its definition files, but if some libraries do this, then that's a problem too. But just looking further up the filesystem does not take care of the case I have reported.

And there are other scenarios that don't involve node_modules. For instance, I could have a project with the following structure:

src/
  tsconfig.json
  a.ts
  b.ts
subunitX/
  x1.ts
  x2.ts

Files in src refer to files in subunitX. tsc and tsserver are both perfectly happy with a structure like this. When a file in src refers to a file in subunitX, they treat the files in subunitX as being part of the same project as the files in src, governed by src/tsconfig.json. If I follow a reference in tide, however, the files in subunitX that tide opens in Emacs will belong to no project.

And for even more fun, you could imagine another subdirectory named otherSrc which is a sibling of src with its own tsconfig.json and containing files referring to files in subunitX. Again, tsc and tsserver will have no problem with this. The files in subunitX, when accessed from files in otherSrc, will belong to the project defined by otherSrc/tsconfig.json. tide will be confused.

@ananthakumaran The file to #256 would solve the problem partially. There would be only one tsserver launched. However, unless changes are made to tide, tide would open in Emacs the files in node_modules as part of a new empty project. So tide's treatment of the file will still not correspond to the compiler's treatment. (Tide may still raise an error on Promise like in the example I gave.)

I still haven't formed an opinion as to that the solution should be.

[josteink]

typedoc (which I contribute to) is planning to use Lerna. I also have projects that I'm thinking of moving to Lerna.

Fair enough. My point was simply that optimizing for Lerna-projects only seemed like the wrong thing to do.

I think you misunderstood the details of the problem.

That may very well be. You're usually much well versed in things once you comment than I am, so I'll take your word for it.

I was just suggesting there might be other, simpler solutions, but if they are indeed too simplistic I see no reason to further pursuit those ideas.

[josteink]

Old issue is old. Closing! :)