atom / ide-typescript

TypeScript and Javascript language support for Atom-IDE
MIT License
368 stars 63 forks source link

Outline view behaves inconsistently #21

Open wyqydsyq opened 7 years ago

wyqydsyq commented 7 years ago

This is a pretty broad issue but that's because the behaviour is broadly inconsistent, I can't find any particular pattern that causes this. I'll be digging deeper and adding detail to the issue or comments as I go.

OS: OpenSUSE Tumbleweed Atom: 1.21.0-beta0 ide-typescript: 0.6.1

Sometimes I get outlines. Sometimes I get No outline available\n There are no outline providers registered. Sometimes the provider just sits in a pending state, showing the spinner. Sometimes if I re-open a file that got stuck pending, it will suddenly change to show the outlines. Sometimes if I re-open a file that had outlines visible, it gets stuck pending.

damieng commented 7 years ago

On larger projects it does take a little time for the typescript server to finish creating its indexes. We need a better way of exposing that it's still doing that - kind of hard as there are no messages in the protocol or the server that indicate it.

One thing you can do is switch on protocol debugging from the dev tools console window with atom.config.set('core.debugLSP', true) to see the message going back and forth (you'll need to close all TS files or reload the Atom window to start the server up again)

Outline view is the "documentSymbols" request so you should see one being sent... and then hopefully shortly after you should see one being received (it will also say how long it took in milliseconds next to it).

wyqydsyq commented 7 years ago

Yeah at first I suspected it was just taking a while to index my larger project, but that doesn't explain why sometimes files that displayed outlines correctly start to get stuck in pending or say there is no outline available, even on the smaller project/files.

damieng commented 7 years ago

Hopefully the debug log will display some info. Failing that I can try and check it out myself if you see this on an open source project I can clone.

wyqydsyq commented 7 years ago

I can see that the request for the Symbols is being sent to the typescript server: image

It eventually results in this: image

So it seems an unhandled error is occurring on the typescript server

wyqydsyq commented 7 years ago

So after trying to reproduce this with some larger open source repositories to no avail, it seems the key difference is that my project is set up as a monorepo containing multiple node TS packages, each with their own package.json as well as a package.json at the root level, and it seems whenever I open a file, it checks ALL package.json files in my ENTIRE project.

Is there a particular reason for this? I would imagine that only the closest package.json to the active file should be activated.

image

damieng commented 7 years ago

Hmm, I'm not too sure. This is one of those areas where it goes off into the weeds of the underlying implementation. We might have more luck speaking to the Soucegraph people who created the wrapper around the language server or even the typescript team themselves.

Does the project load okay in VS Code?

wyqydsyq commented 7 years ago

Yeah all my coworkers use VSCode on the same project without issue.

It seems related to the context active in Atom.

If I open the top-level monorepo as the Project Folder, I get this inconsistent (or non functional) behaviour on files in the subprojects. If I open one of the subproject directories as the Project folder, it behaves as expected.

Here I've opened a subproject directory as the Project Folder, and opened logger.ts, all good: image

Here I've opened the exact same file, except in an Atom instance where the monorepo root is the Project Folder, it gets stuck in pending: image

So it appears likely to not be an issue with the ts server, rather the context / details being sent to it from this Atom plugin. It seems that the plugin is sending the context of the Atom Project, rather than the "Node Project", which would be the closest ancestor directory to the active file with a package.json

damieng commented 7 years ago

Hmm, I wonder if VSCode opens up multiple language servers - one for each folder and if they do what heuristic they use to decide that.

On Tue, Sep 12, 2017 at 9:29 PM Damon Poole notifications@github.com wrote:

Yeah all my coworkers use VSCode on the same project without issue.

It seems related to the context active in Atom.

If I open the top-level monorepo as the Project Folder, I get this inconsistent (or non functional) behaviour on files in the subprojects. If I open one of the subproject directories as the Project folder, it behaves as expected.

Here I've opened a subproject directory as the Project Folder, and loaded opened logger.ts, all good: [image: image] https://user-images.githubusercontent.com/621277/30359659-a18b7812-988f-11e7-8b55-d3e949c603fb.png

Here I've opened the exact same file, except in an Atom instance where the monorepo root is the Project Folder, it gets stuck in pending: [image: image] https://user-images.githubusercontent.com/621277/30359698-d394de52-988f-11e7-910c-078437d9598d.png

So it appears likely to not be an issue with the ts server, rather the context / details being sent to it from this Atom plugin.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/atom/ide-typescript/issues/21#issuecomment-329055649, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHQp4p3UYBNUV1prIgGxQuKRCqpRCbJks5sh1oigaJpZM4PVi8d .

luketheobscure commented 7 years ago

Related - just tried this on a large Ember project, and the language server churned for an extended period chewing through the tmp folder.

Seems like it needs to either be set up to ignore hidden files and folders, or have the option to manually exclude.

wyqydsyq commented 7 years ago

I believe this module should be respecting users' tsconfig.json files, only files matched by the include (and not excludeed) option of the active tsconfig.json file (already being read for Diagnostics I believe) should be sent in the context to the typescript language server. The module is currently trying to send my whole entire Atom project to the language server which seems to break it due to either the sheer volume or by it not being able to understand the nested structure of my project.

damieng commented 7 years ago

I believe it is respecting the tsconfig.json - but it's quite likely also only expecting a single one to exist in the root folder you open.

For us to handle multiple nested tsconfig's we'd need to open multiple servers - this is probably what VS Code does as well.

wyqydsyq commented 7 years ago

Yeah that's what I meant sorry, it should treat tsconfig.json the same way that tsc does - finding the closest ancestor (or manually specified one) and using that, rather than just sending all tsconfig.json files it finds to the language server.

What I'm asking about here isn't so much handling nested tsconfigs via concurrently interpreting all of them, rather handling nested tsconfigs by ignoring all of them except the relevant one. In typescript only one tsconfig is ever used at any given time (excluding cases where a tsconfig is specified that extends another one).

So say I had this project structure:

/my-cloud
 - /src
 - /tsconfig.json
 - /services
    - /client
        - /src
        - /tsconfig.json
    - /admin
        - /src
        - /tsconfig.json

Currently if I were to open a file such as /my-cloud/services/admin/src/index.ts, this plugin seems to be sending all tsconfig.json files it can find in my Atom project to the ts server.

I believe the correct behaviour would be for it to only send the context of /my-cloud/services/admin/tsconfig.json to the ts server, because that is where the "active" tsconfig.json for the open file index.ts is. If I were to open /my-cloud/services/client/index.ts, the ts language server should receive the context of only /my-cloud/services/client/tsconfig.json. Other tsconfig files are irrelevant to the context of the open typescript file.

I'm not sure if this approach of sending the context upon activating (focusing) a file would be viable, as the plugin seems to currently send context when you first open a file rather than activating the tab, but I think it would be a better alternative than starting multiple language servers in order to parse the extra tsconfig files which aren't even relevant to the file being edited, which I think would be wasteful because you're starting language servers and having them read all these files which ultimately do not get used at all in providing information for the relevant file.

damieng commented 7 years ago

tsc is the language server so it treats them the same way. We don't send any tsconfig files. tsc picks them up itself.

I can imagine firing up a separate language server for admin and client in your example as they both have a tsconfig.json... i'm not sure what VSCode though does if you open my-cloud as it also has one.

wyqydsyq commented 7 years ago

But in a situation where I am editing a file under client, what purpose would firing up a language server for the admin part serve? If I'm editing a file under client, tsc doesn't need to know about everything in admin. Because everything it refers to or depends on can be determined from it's source or it's own package.json

I think I incorrectly fixated on referring to tsconfig being used for the context, it seems to be actually using package.json for the context: image

So here you can see it's passing two package.json files to the server. What purpose does this serve when you're only interested in the results that are relevant to the file you're currently editing? If I'm editing a file in the top level /unity directory, why are we sending the package.json for automation_tests as well?

damieng commented 7 years ago

It wouldn't. I'm saying we'd fire up a language server that just points directly to the closest folder above when you open a file. i.e. you open client/a/b/c.ts we'd scan up until we hit client/tsconfig.json then fire up a language server for client.

The real question is what to do if you open a file above client, say in /src/ in your example. We'd need to fire up a server for my-cloud which would appear once again to include all the sub-folders. Having a tsconfig.json at that level causes some confusion. We'll need to dig in to what other clients do.

It's made a little more opaque in that we use a third party wrapper around tsc to make it speak language server protocol as Microsoft haven't added support for the protocol directly to tsc yet.

Those log messages do not show us passing two package.json files to the server - they are messages from the server about what it's found. The initialize message we send contains just a path the server should operate on - we send no further information about the file system other than letting them know what files we are opening and closing, renames and deletes.

damieng commented 6 years ago

I think for this to be effectively resolved the wrapper we use around tsserver to expose it as a language server would need to look for tsconfig.json and jsconfig.json files beneath the hierarchy and fire off a tsserver for each. We'll look into what can be done upstream here as it affects a lot of scenarios.