Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make cross-file go-to-definition work in the syntax-only server #39037

Closed
DanielRosenwasser opened this issue Jun 12, 2020 · 8 comments · Fixed by #39476
Closed

Make cross-file go-to-definition work in the syntax-only server #39037

DanielRosenwasser opened this issue Jun 12, 2020 · 8 comments · Fixed by #39476
Assignees
Labels
Bug A bug in TypeScript Rescheduled This issue was previously scheduled to an earlier milestone

Comments

@DanielRosenwasser
Copy link
Member

One of the original motivators for the syntax-only server was to make sure that users could navigate around their project, and inter-file go-to-definition is table-stakes. Some of this is already described in #37713, but the idea is:

// @filename: foo.ts
export interface Stuff {
  foo: string;
}

export function foo(x: Stuff) {
}

// @filename: bar.ts
import { foo } from "./foo";
import * as fooModule from "./foo"

/**/foo()

fooModule./**/foo();

Go to definition on either call to foo should just work. Go to definition on the import path should also work. We can do this by performing a super-basic basic resolution of the path.

If there's an export *, we should consider trying to continue the search too.

@sheetalkamat
Copy link
Member

Investigated this on how to go about it..
Instead of doing some magic on the fly, whenever someone asks for LS for semantic operations, create a project that either resolves relative imports only in open file (potentially whole tree with only relative import/export resolutions). (and we can do this when someone asks for completion/quick info instead of when opening the file)
I think this might be simplet than cooking up goto def/completions on the fly.

@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented Jul 1, 2020

Does that also mean that we'll get slightly more accurate type information?

For example, if checker.ts is a module, and types.ts is a direct dependency, then in that case would every type reference be resolved?

(CC @minestarks since you had thoughts around different scenarios)

@sheetalkamat sheetalkamat added the Fix Available A PR has been opened for this issue label Jul 8, 2020
@DanielRosenwasser DanielRosenwasser added the Fixed A PR has been merged for this issue label Jul 16, 2020
@DanielRosenwasser DanielRosenwasser removed Fix Available A PR has been opened for this issue Fixed A PR has been merged for this issue labels Aug 19, 2020
@RyanCavanaugh RyanCavanaugh added the Rescheduled This issue was previously scheduled to an earlier milestone label Dec 11, 2020
@sheetalkamat
Copy link
Member

@DanielRosenwasser @RyanCavanaugh do we really want this.. we have not received any feedback on partial mode so its hard to tell if its worth the extra work that needs to be done and what all scenarios can be made to work and what is limitation...

@DanielRosenwasser
Copy link
Member Author

Well that was the original motivating scenario for some teams, that eventually produced partial mode itself. @brieb and @unindented might be able to give a sense of whether cross-file go-to-definition on startup currently feels like a bottleneck, but given that we now are expected to run beyond just in the desktop, there might be more thought that needs to go into this.

@unindented
Copy link

unindented commented Dec 12, 2020

cross-file go-to-definition on startup currently feels like a bottleneck

It does take a while for our type definitions to load on startup (~30s), so if you need to navigate immediately after starting the editor it gets a bit annoying.

I also see type definitions take a while to load after navigating across projects by Cmd + clicking references, and then going back with Ctrl + -. I can't repro consistently, so it must be something about some type definitions getting evicted from memory after a while.


Just for my understanding, partial mode is that behavior where Cmd + clicking a reference whose type definitions haven't loaded yet tries to take you to a useful place in the same file?

@sheetalkamat
Copy link
Member

Ok will investigate this in 4.3 to see how we can improve this.

@brieb
Copy link

brieb commented Jan 6, 2021

Great, thank you so much for investigating! (And happy new year!)

Yes, anything that improves the editing experience while booting up large projects is very welcome and helpful. In lieu of our project startup itself being fast, the introduction of this partial editing mode has helped engineers stay productive with the most common editing tasks while things initialize.

In the current experience for us, going to definition on an imported symbol first navigates to the import statement. Then going to definition again finds the local usages in that file (see screenshot below for demonstration). That behavior is less intuitive than navigating to the imported file (which is what happens once the server is fully initialized).

image

@sheetalkamat
Copy link
Member

#42539 handles jumping to the files.. #38836 to track handling unresolved locals/members

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Rescheduled This issue was previously scheduled to an earlier milestone
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants