-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Find Source Definition #48264
Find Source Definition #48264
Conversation
This reverts commit 381799d.
Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary |
One case to consider as we think about how this functionality should be triggered is what ctrl-click should do. |
@typescript-bot pack this |
Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 34c6cfd. You can monitor the build here. |
This reverts commit 34c6cfd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just need to make some of the methods internal
Latest commits
|
* Prototype resolving to JS when go-to-def aliases all resolve to ambient declarations * Add test infrastructure * Start fleshing out test coverage * Fix some go-to-def stuff * Finish lodash test case * Make go-to-implementation never return ambient results * Build new functionality into go-to-implementation * Update baselines * Two more test cases * Refine definition searches for unresolved imports * Revert "Build new functionality into go-to-implementation" This reverts commit 381799d. * Fix tests * Revert go-to-implementation changes * Wow a bunch of code was unnecessary * Update baselines and go-to-def test * Fix navigation on symbols that are not aliases but resolve through aliases in chain * Temporarily replace go-to-def with new command implementation * Revert "Temporarily replace go-to-def with new command implementation" This reverts commit 34c6cfd. * Revert "Wow a bunch of code was unnecessary" This reverts commit 1cb2ba6. * Bring back some deleted code needed for a new test case * Clean up a little * Rename more stuff * Update test * Update API baseline * Temporarily replace go-to-def with new command implementation * PR review fixes * Fix getTopMostDeclarationNamesInFile * Rename local * Use hash set * Remove option from commandLineParser * Keep noDtsResolution project around * Handle AuxiliaryProject kind in ScriptInfo getDefaultProject etc. * Do not run updateGraph in the background for AuxiliaryProject * Don’t create auxiliary project outside of semantic mode * No-op on scheduled invalidation * Add comments to unit test * Sync compiler options to auxiliary project * Fix case sensitivity * Update extensionIsOk with new file extensions * PR feedback * Update API baseline * Mark scheduleInvalidateResolutionsOfFailedLookupLocations internal * Use same heuristics on property accesses of loosely-resolvable aliases as unresolvable named imports * Rename command, and no need to return the bound span * Update API baseline
Could we expose |
Unfortunately not—if you look at the implementation here, Find Source Definition happens by orchestrating multiple different language services to call Go To Definition with different module resolution options. What are you hoping to integrate it into? |
@andrewbranch I hope it can use in embedded typescript language for Vue. (For example: https://github.com/johnsoncodehk/volar-starter/blob/e4f6d9808181f4533811c1aafc4b6afea1f68598/src/components/HelloWorld.vue#L26-L30) For
I'm also expected a new (btw |
It’s simply not possible to put this functionality on the LanguageService object (or, it would be a significant violation of the LS’s scope of concerns). At minimum you would need to create a second language service (which creates a second Program and TypeChecker!). It would take a bit of work on our end to abstract what I put directly into TS Server into some kind of public API that could be used independently of the project system. This is an experimental API so it’s not going to happen right now, but if you want to open a new suggestion issue for doing this, we’ll keep an eye on it and could discuss some of the design challenges. Thanks! |
Actually this is absolutely acceptable 😅, we already have multiple language server & language service instances in order to prevent auto-complete request from being blocked by unfinished diagnostics request. (vuejs/language-tools#393 (reply in thread)) Great to see it experimentally added to the language service API, this feature will be very welcome in the Vue community, it solves some common DX issues in Vue projects (For example: vuejs/language-tools#1226). It doesn't matter if it will be removed anytime in the future, I can adapt the code quickly.
Of course, thanks in advance! |
This PR adds a new command similar to Go To Definition, but includes locations in JS files that would normally not be considered, excludes all ambient locations, and potentially uses some heuristics to find likely results in JS files that cannot be analyzed with our binder and type checker due to complex module wrappers or other constructs we don’t understand. The process when invoked is:
I attempted to tack this onto go-to-implementation instead of its own command, but go-to-implementation is extremely weird and did not play nicely with this code. This is much more go-to-definition-like, but I don’t think we can simply augment or replace go-to-definition because we want go-to-def to be super fast and reliable, and I think we don’t want this command returning anything ambient. However, for purposes of gathering public feedback in the meantime, I think I’m going to make a build that replaces go-to-def with this logic. (Otherwise, users would have to use a special build of VS Code in order to send the new command.)
Fixes #6209
Fixes #38474