TypeStrong / atom-typescript

The only TypeScript package you will ever need
https://atom.io/packages/atom-typescript
MIT License
1.13k stars 205 forks source link

@types declarations not reloaded on change, if tsconfig is in a subdirectory (and node_modules isn't) #1486

Closed Neonit closed 4 years ago

Neonit commented 5 years ago

When a declaration file in my project changes, atom-typescript won't reload it unless I reload the editor. Related to https://github.com/TypeStrong/atom-typescript/issues/915.

@basarat posted a solution there, which worked fine for a long time, but since I updated to 12.7.3 it does not work anymore.

In own words: Previously, if a declaration file changed, i just opened it in the editor and atom-typescript immediately became aware of the changes and refreshed its cache. Now this doesn't work anymore. I have to reload the editor (Ctrl + Shift + F5).

Please unremove the feature.

Neonit commented 5 years ago

My current workaround (much faster than reloading the editor):

  1. Ctrl + Shift + P (command-palette:toggle)
  2. Typescript: Restart All Servers

(or bind this of course)

lierdakil commented 5 years ago

Sorry, I was under the impression I replied, but apparently not.

TypeScript server can sometimes get "stuck" with an inconsistent state -- that was the primary reason for the introduction of typescript:restart-all-servers command (because of course reloading the editor is a pain).

That said, with declaration files, opening the declaration file in question and then saving it, or running typescript:check-all-files generally should force tsserver to reload the declaration. If this doesn't work for you and you can construct a minimal testcase, it'd be very helpful if you shared that, because I can't really reproduce this at all. Currently on my machines tsserver v3.2.2 automatically picks up changes in declaration files even if those are changed from outside Atom: Animation showing automatic update of declaration files

Neonit commented 5 years ago

Thanks for the response.

This appears to be a weird problem with my older projects. Let's call the projects this problem occurs for A and B. Here is what I tried so far.

So, I don't know what causes this, but it's maybe something carried over from projects that were using older versions previously? Is there a cache or the like? Does atom-typescript read the package.json or package-lock.json? I am hoping some detail in what I described above rang a bell in the head of somebody knowing a bit about the internals. I'm using gulp and file watchers as well, which shouldn't be relevant, but could it be that something installed a buggy/outdated file watching package now also erroneously used by atom-typescript?

For now I cannot provide a way for reproduction. I can't publish my whole project. Using Windows 7 x64 by the way. Open for suggestions on what to try out.

EDIT I just noticed I don't even need to change the declaration file externally. Even if I have it open in the same Atom instance, change and save it the changes are not shown in the importing file.

Neonit commented 5 years ago

I found out how to reproduce my case with a very minimal example with the following file tree:

+ project
  + node_modules
    + @types
      + foo
        + Foo.d.ts
  + src
    + bar.ts
    + tsconfig.json

node_modules/@types/foo/Foo.d.ts:

export declare const foo = 42;

src/bar.ts:

import {foo} from 'foo/Foo';
const bar = foo / 2;

src/tsconfig.json:

{
  "compilerOptions": {
    "types": []
  }
}
  1. Open Atom in the project folder and the files node_modules/@types/foo/Foo.d.ts and src/bar.ts.
  2. There should be no errors or warnings.
  3. In the Foo.d.ts change 42 to '42' (or any other value not allowed with the / operator) and save.
  4. Switch to bar.ts. There should be no error displayed, although there should should (you know what I mean). Even reopening the files in Atom doesn't change anything. The editor keeps displaying foo as constant with 42 as number value. Saving the files doesn't help either. F12-ing foo doesn't help. What helps is restarting the TS servers or reloading the editor.
  5. Removing the src/tsconfig.json or moving it to the project root makes the editor behave different. In that case the declaration will be refreshed once open and the error checking will be repeated on file save, which is the intended and described behavior (although I find it's too lazy; I would rather have the editor auto-reload and auto-recheck on change of any imported file).

I don't know about general opinions or do's and dont's concerning nested tsconfig.jsons, but I cannot work without them, because I have both Node.js and browser ends in one project. I need it like that, because I also have a shared codebase, which gets compiled for different targets.

lierdakil commented 5 years ago

Thanks for investigating. But I have to ask.

Why is types in tsconfig.json empty? That might be the reason for the strange behaviour. Try either removing the types directive altogether, or adding ../node_modules/@types there?

Neonit commented 5 years ago

I was just too lazy to put a default tsconfig.json there so I just filled it with the minimal things I need. I need a tsconfig.json there so the issue can be reproduced. Further:

I think for ../node_modules/@types types would be the wrong place, wouldn't it? This should, if at all, be set with typeRoots. Doesn't fix the issue, though. To me it appears Typescript correctly finds the @types in the node_modules, even if it has to look for them a directory up of the tsconfig.json.

lierdakil commented 5 years ago

would be the wrong place, wouldn't it?

Right. I misremembered that, sorry.

I was able to reproduce using your guide. After a bit of investigation, here's what's going on here:

When you open node_modules/@types/foo/Foo.d.ts, Atom-TypeScript tries to assign it to a project by searching the directory tree upwards for any tsconfig.json. A separate tsserver instance is spawned for each tsconfig.json to avoid madness that results otherwise when the same file is referenced by multiple projects (long story short, the active project for such a file will be arbitrary), and one for files with no project. Since there is no tsconfig.json in the directory hierarchy leading to Foo.d.ts, the newly-open file is treated as having no project, and so nothing you do with Foo.d.ts here will affect the tsserver instance in which bar.ts is loaded (which is correctly assigned to src/tsconfig.json project)

On the other hand, tsserver correctly reports Foo.d.ts as a part of the src/tsconfig.json project when running check-all-files or build. However, it apparently fails to watch the file for changes. I have no idea why that is. I think I've seen it work as expected (i.e. tsserver watching @types for changes), while here it for some reason does not, but maybe I'm mistaken. This might be a bug in tsserver, or perhaps a deliberate choice since @types declarations aren't supposed to be changing all that often.

So, I really don't want to revert the code that sidesteps the multiple-projects-one-file woes, but it apparently does create issues, and the case it was supposed to fix is not all that common, so I will hide that behind an option. See if v13.1.0 behaves more like you would expect. Note that you still have to open the declaration file for it to be updated -- unless file-watching is changed in the tsserver, not much can be done about that.

Thanks for your help in tracking this down!

Neonit commented 5 years ago

Thank you for caring. 💙 (BTW you forgot to remove the unable-to-reproduce tag, if that matters)

After thinking about this a few times with all these changes and a shortcut for restarting the TS servers setup as a workaround I came to the conclusion that it's much quicker to restart the TS servers, when a lib changes than opening the declaration file and saving my current file. In the past saving (or anything else to force a re-check) was not necessary.

And funnily my project is one of the not at all that common cases, I think. I have a folder with shared sources, which have an own tsconfig.json (for live checking) and are used by two other subprojects, one for Node, one for browsers.

As I read your explanation about how things work in the background, an approach I would have taken for handling things popped up and I wondered why it's not done like that. For each open file I would maintain a list of referenced (or dependency) files and watch these. Am I thinking too naive?

lierdakil commented 5 years ago

you forgot to remove the unable-to-reproduce tag, if that matters

Not hugely, but thanks for pointing it out.

For each open file I would maintain a list of referenced (or dependency) files and watch these. Am I thinking too naive?

Okay, so tracking dependencies is not that simple, and since those can change arbitrarily and quickly, setting up and tearing down file watchers can be a bit costly, both in terms of code complexity and system resources.

The more straightforward option is setting up watchers for all project files -- which are relatively easy to track most of the time and those don't change as often. The caveat is, tsserver is supposed to do that already -- and it does, with a notable exception of @types (or, perhaps more generally, typeRoots, not entirely sure). Reimplementing that logic in Atom seems unjustified -- it's breaking the "single responsibility" rule, and it can, if not implemented very carefully, create a hard-to-track race conditions. So not something I'd be too stoked about, frankly.

I guess the question is, why doesn't tsserver watch @types declarations for changes? If it's a bug or an oversight, that should most likely be fixed on TypeScript side, instead of creating brittle workarounds on Atom side. If it's a deliberate design decision, there has to be a reason for that -- and I'd want to know it before an extended trip to the workaround-land. I did try to search for related issues on the TypeScript repo, but didn't find much, and sadly I don't have a ton of time to go through the code currently. Perhaps, at this point, creating an upstream issue might be justified.

github-actions[bot] commented 4 years ago

This issue has been marked as stale because it did not have any activity for the last 90 days or more. Remove the stale label or comment or this will be closed in 14 days