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

[typescript-language-features] Support replacing Go to Definition with Go to Source Definition by preference #178840

Merged
merged 3 commits into from
Apr 5, 2023

Conversation

andrewbranch
Copy link
Member

When a new preference is enabled, Go To Definition shows definitions from Go To Source Definition when available, instead of its typical results.

This is achievable without TS Server changes, allowing it to work on TypeScript versions as old as 4.7, but perhaps not optimally so. As it is now, definitionAndBoundSpan is still called first, because findSourceDefinition does not return the text span used to draw the underline, and it also does not return fallback results to type declarations, which I think we would want if Go To Definition is fully replaced. findSourceDefinition is called subsequently, and the definitions it returns replace the ones returned by definitionAndBoundSpan. If findSourceDefinition returns no results, the response from definitionAndBoundSpan is used as a fallback.

definitionAndBoundSpan is typically very cheap, so this might be acceptable. But there are two ways we could improve performance:

  1. Make a new TS Server command that returns source definitions, along with the text span, and falls back to type declarations when no other definitions are found. This would allow VS Code to trigger only one request, provided the TS version is new enough to have this capability.
  2. A bigger change to VS Code, but the best thing for perceived performance would be to support separate requests for drawing the underline and showing the definitions. findSourceDefinition can be considerably more expensive than definitionAndBoundSpan, so it would be nice to trigger the latter when hovering with ctrl/cmd, and then trigger the former once the user clicks.

"description": "%configuration.preferGoToSourceDefinition%",
"scope": "window"
},
"javascript.preferGoToSourceDefinition": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] needs formatting

@mjbvz mjbvz enabled auto-merge (squash) April 5, 2023 20:56
@mjbvz
Copy link
Collaborator

mjbvz commented Apr 5, 2023

As discussed, we'll merge this as is since it seems good enough. In future TS release, we may update the source definition request to return a span

@mjbvz mjbvz merged commit 1176681 into microsoft:main Apr 5, 2023
@github-actions github-actions bot locked and limited conversation to collaborators May 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants