-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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
Add Go To Implementation Api #18346
Add Go To Implementation Api #18346
Conversation
For microsoft#10806 Adds a new API for supporting `go to implementation` command for languages. Implements an example for TS
Removing myself from reviewer, unless there is a very specific reason why my feedback is needed in this area. Please let me know. |
precondition: ModeContextKeys.hasImplementationProvider, | ||
kbOpts: { | ||
kbExpr: EditorContextKeys.TextFocus, | ||
primary: KeyMod.CtrlCmd | KeyCode.F12 |
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.
conflicts with Find References
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.
Ok. Visual Studio uses alt+f12
for "go to implementation" but we already have a binding for that to editor.action.previewDeclaration
. I don't see any default binding for cmd+f12 though, but am not sure how this would on other platforms. Any suggestions for an unused keybinding that would work on all platforms well?
* The implementation provider interface defines the contract between extensions and | ||
* the go to implementation feature. | ||
*/ | ||
export interface ImplementationProvider { |
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.
Maybe TypeDefinitionProvider
would be a better name?
Good stuff - added some nit comments, ignored the actual implementation in the TypeScript extensions. I would definitely go for a separate provider despite them being quite similar. My name proposal is |
+1 for the separate provider. Name wise I would go with TypeOfDefintionProvider :-) |
Thanks for the review. I've updated the interface names and am looking into the keybindings |
@dbaeumer The language service API should probably pick up support for this feature, but I wanted to check with you to see that should block this PR. I think we can either:
What are your thoughts on this? |
@mjbvz we always ship new API in VS Code and then update the LSP later. Can you please file n issue here https://github.com/Microsoft/language-server-protocol so I don't forget to do it. |
**bug** In microsoft#18346, I originally called the new go to implementation provider api `ImplementationProvider` which we then decided to rename to `TypeDefinitionProvider`. At the time, I didn't realize that a type definition was actually its own, unrelated concept. **Fix** Rename `TypeDefinitionProvider` to `TypeImplementationProvider` to make it clear what the purpose and use of this api is.
* Rename TypeDefinitionProvider to TypeImeplementationProvider **bug** In #18346, I originally called the new go to implementation provider api `ImplementationProvider` which we then decided to rename to `TypeDefinitionProvider`. At the time, I didn't realize that a type definition was actually its own, unrelated concept. **Fix** Rename `TypeDefinitionProvider` to `TypeImplementationProvider` to make it clear what the purpose and use of this api is. * Fix a few names in comments
Fixes #10806
Adds a new API for supporting
go to implementation
type commands in languages. Implements an example for TSThe current approach added a new public interface called
ImplementationProvider
(a suggestions for a better name would be appreciated). This resulted in some duplicate code that I will try to clean up further. An alternate design would be to extend theDefintionProvider
with an optionalprovideImplemtnation
method. This would allow reusing our existing internal interfaces without as much duplication, but we may want to keep the two concepts separate. Please let me know if you have any thoughts one way or the other.