Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Outline view behaves inconsistently #21

Open
wyqydsyq opened this issue Sep 13, 2017 · 16 comments
Open

Outline view behaves inconsistently #21

wyqydsyq opened this issue Sep 13, 2017 · 16 comments

Comments

@wyqydsyq
Copy link

wyqydsyq commented Sep 13, 2017

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
Copy link
Contributor

damieng commented Sep 13, 2017

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
Copy link
Author

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
Copy link
Contributor

damieng commented Sep 13, 2017

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
Copy link
Author

wyqydsyq commented Sep 13, 2017

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
Copy link
Author

wyqydsyq commented Sep 13, 2017

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
Copy link
Contributor

damieng commented Sep 13, 2017

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
Copy link
Author

wyqydsyq commented Sep 13, 2017

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
Copy link
Contributor

damieng commented Sep 13, 2017 via email

@luketheobscure
Copy link

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
Copy link
Author

wyqydsyq commented Sep 15, 2017

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
Copy link
Contributor

damieng commented Sep 15, 2017

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
Copy link
Author

wyqydsyq commented Sep 15, 2017

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
Copy link
Contributor

damieng commented Sep 15, 2017

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
Copy link
Author

wyqydsyq commented Sep 15, 2017

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
Copy link
Contributor

damieng commented Sep 15, 2017

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
Copy link
Contributor

damieng commented Feb 20, 2018

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants