-
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
Go To Source Definition feedback thread #49003
Comments
Found a crash in an inferred JS project when using a destructured declaration on a // index.js
const { ncp } = require("ncp"); // package.json
{
"name": "yadda",
"version": "1.0.0",
"description": "Yadda",
"main": "index.js",
"dependencies": {
"ncp": "^2.0.0"
}
} TSServer Crash
|
@DanielRosenwasser can you give me compiler options for that? Also, does go-to-def jump to your ATA cache? |
Oh, missed this. |
Typescript 4.7 and vscode insiders has Go To Source Definition if the declarationMap is enabled. microsoft/TypeScript#49003 This would make packages larger via the `.d.ts.map`
would enabling |
Yes.
What is “this” in this context? And to be clear, declaration maps have always been used for Go To Definition when available—nothing new about that in 4.7. rxjs is an example in the wild—a plain old Go To Definition on anything you import from there will go to the TS source. |
I was confused because the packages I was trying with "go to source definition" weren't working. I think this is because I had |
Hey @andrewbranch great work on this, I had a quick Q (you kind of mentioned it in your original comment). Shouldn't "Go To Definition" be the default for this and there should be a separate command for going to the type definition? I only ask this because it will be inconsistent with other languages being used in VSCode where "Go To Definition" would go to the original source definition of that symbol. I feel like most people using the "Go To Definition" command are expecting it to go to the source code not the type, so I was surprised to see the solution was not to change it but instead add a new command. This will end up making TS/JS a "special snowflake" in the VSCode UI where you need to navigate to another command to get the same result that you'd have from other languages. I could be wrong but that seems to go against VSCode's goal to have familiar UX across all languages |
I think I answered that in this Twitter thread. Let me know if that leaves you with any more questions. |
Ok thanks @andrewbranch I understand this is not wanting to change/break workflow for those who are already used to it. |
Well there’s also the fact that Go To Type Definition means something completely different, and Go To Definition is kind of sacred in that it shows you with 100% confidence what the compiler thinks the definition of some code is. Keep in mind that Go To Definition does take you to the original, concrete source when it can do so with 100% confidence—that is, when declaration maps and original TS sources exist. It would be great if this were always true, but the reality is that TS is a bit of a special snowflake in that our “header files” and “implementation files,” if you want to extend the analogy to other languages, are often written, by hand, by different parties at different times under different versions installed to different locations and the extent to which they match each other is determined solely by the skill of the declaration author and the ability of the user to install matching versions of each (I’m referring to getting .d.ts files from DefinitelyTyped, of course). Such uncertainty deserves to be treated differently than the distinction between a header and implementation file compiled from a single source. And also/again, types are so central to the process of writing TypeScript that even if we could always take you to the right spot in the JS with 100% certainty, I still don’t think we’d call it the “definition” (at least for TS users). |
When using this feature for an imported Typescript library ( If my guess is right: could we have a better message please? Maybe 'Typescript 4.7 required in order to support Go To Source Definition'? Or probably better: disable the feature until the environment can support it? |
@jzabinski-dolios VS Code does hide the menu item if it didn’t see TypeScript 4.7 in the workspace, and gives this error message if you invoke it by keyboard shortcut anyway: Unless something really weird is happening, I would guess you’re actually using VS Code’s bundled version of TypeScript (now 4.7) in the workspace, and we really couldn’t find the source for that import due to one of the limitations described in the issue body. |
Glad to see this finally made it in. Could we get an option to exclude folders from the search? Specifically mine keeps linking to /build/ files, when /src/ files are there in original JSX form. Would love to exclude This should both help me narrow my own needs, but also make it more performant. |
@danieliser which package is doing that? Can you provide a repro? The answer to your question is very likely “no.” We found One neat example of how this can work well is React. Its entry point sets |
It would be nice if I could tell VSCode where the source to go to is. Currently VSCode jumps to the installed and transpiled TypeScript package. The caveat is that you could not fully understand neither the However I have clone the original source of the installed package on my disk. So I could tell VSCode where to go to instead and browse the original TypeScript files. I know a similar feature (~15yrs ago) from Eclipse, where you could tell the IDE where to find the source of a Java package. Instead going to the compiled sources, Eclipse jumped to files in the package. |
What's the best way to jump to the typescript definition, the We're in a typescript monorepo, and cmd-clicking on imports between packages in the same monorepo results in the |
@benhickson you need to enable declaration source maps for that to happen |
Today, I tried out the "Go to Source Definition" feature with my NX monorepo. (NX is quickly becoming one of the most popular TypeScript monorepo tools.) In an NX monorepo, the
This is a break from the more conventional monorepo layout, where each
NX changes this convention for a good reason - so that the Unfortunately, the consequence of this change is that the
With that said, manually merging source directories with dist directories in a build script is a little clunky, and requires technical knowledge of how all of these working parts plug together. A much better solution would be a compiler flag. Currently, TypeScript offers the This feature is kind-of-related to #30835, which is closed as wont-fix. With that said, I think that if the TypeScript team is going to introduce "Go to Source Definition" as an official TypeScript feature, it would be really great to also offer a solution to make it work with minimal hassle for everyone. |
Sounds like this is just a bug with |
Andrew, In this case, TypeScript can't map to the source files, because they don't exist. See the directory structure map that I sketched out above. For example, say that I run
It creates the following files:
The contents of
In this case, the root directory of the uploaded files to npm are
Let me know if that isn't clear and I can make an example repo. |
The plan is actually to make that message less prominent: microsoft/vscode#152252 But we can put more info about what happened in the TS Server log. |
Hello, thank you for posting this feedback issue. At the moment, it seems like many TypeScript library authors only include definition files and compiled, minimized js files. Are there any processes in motion to improve this situation? Is there a change needed in the NPM packaging process or in the TypeScript compiler? It would be amazing for the dev experience if in the common tools, it would be the default to include source code and declaration maps. All of the publicly available NPM packages are open source so it's not like we can't read the source already and library authors could still turn off this functionality if they have specific reasons to do so. |
I would love this to be the behaviour when I use Ctrl + Click. In my experience the current behaviour of Go to Definition is pretty useless. This new Go to Source Definition option fixes that. I kind of understand why this isn’t the default behaviour from reading previous comments, but I don’t see why this behaviour can’t be opt-in. |
⌘-click => Go To Source ❤️ |
This is already possible via declaration maps, assuming the library authors correctly configure their minimizers to also emit source maps. |
GtS is fantastic! ❤️ Thanks! Two more motivating use cases for optionally mapping
|
(Not feedback. Just creating some links on the web) Related Stack Overflow question: How can I have Visual Studio Code ignore .d.ts files when I reference a definition and take me to the JavaScript source definition? |
How can i get this to work in a .svelte file? right now it displays "Go to Source Definition failed. unsupported file type" |
TypeScript provides an API for this. VSCode implements this for TypeScript. For this to work with Svelte / MDX / Astro / any other TypeScript based IntelliSense, their extension needs to implement this. |
I only get "No source definitions found" with most library functions. Is there a reason why it almost never works? |
@wvhulle the post contains instructions about what to do if it's not working for you |
Hi thanks, but read through this whole thread and couldn't find anything. I've opened the typescript server output but didn't see anything. Which post do you mean? |
|
@wvhulle Have you read this topic? #49003 (comment) |
I have issues with the libraries vite and sveltekit. Would this be because of the way they are packaged? |
As of today’s VS Code Nightly, you can enable a setting that replaces Go To Definition with Go To Source Definition, allowing you to ctrl/⌘-click to trigger it. If source definitions cannot be retrieved, it falls back to Go To Definition. Since this addresses the only piece of feedback I’ve consistently received, I’m going to close this issue. Thanks everyone! |
Another related Stack Overflow question: Go to implementation instead of TypeScript declaration (where I've edited my answer post to mention the new "Prefer Go To Source Definition" setting). @andrewbranch If possible, please also get microsoft/vscode#82054 temporarily reopened to mention this new setting and then close it again. |
hi, i am new to typescript, and recently study vue-typescript-admin-template i meet a issue, when disable typescript "Prefer go to source definition", cmd + click jump to @types node_modules' but when i enable |
@toien would you be able to file a new issue with specific repro steps? See the “Make sure to include” list at the very top of this issue. Thanks! |
Different flavors of this get a little bit out of hand, I guess... but what I really miss (or I don't know how to achieve...) is being able to jump to the runtime source when the original source (or dts) is available. I'm in a JS file and I have real problems when it comes to jumping straight to a sibling JS file ( const upload_1 = require("./upload"/*1*/); No matter what I do here, I'm being brought to |
When the source code is available it should be preferred instead of following type definitions. This commit also moves the "ftplugin" directory to "../after" to follow kickstart.nvim's implementation. References: - [Go To Source Definition feedback thread](microsoft/TypeScript#49003) - [tsserver's goToSourceDefinition](https://github.com/typescript-language-server/typescript-language-server?tab=readme-ov-file#go-to-source-definition)
@fabOnReact |
Seeding a thread for feedback on / issues with Go To Source Definition (#48264) ahead of the 4.7 release.
TL;DR
TypeScript 4.7 (already in nightly) and VS Code 1.67 (already released) will contain a new navigation command called “Go To Source Definition” that attempts to find a definition in non-ambient TypeScript or JavaScript code. (Ambient code means
.d.ts
files ordeclare
contexts inside regular TS files—in other words, definitions separated from concrete implementation.) The most common reason to use this is if you want to look at the JS implementation of something you imported fromnode_modules
, where Go To Definition only jumps to.d.ts
files.Our ability to find concrete definitions in JS when Go To Definition returns only
.d.ts
files varies wildly based on a lot of complicated stuff. Sometimes this feature returns results that we can be nearly 100% confident in. Sometimes it returns a guess that definitely might be wrong. Sometimes it returns nothing. If it does something that surprises you, I would like to hear about it here, even though it might be a known limitation. Make sure to include:@types
library in your node_modules, and what version it isKnown limitations
The main thing that makes this feature go from 100% confidence to guesses that could go wrong is JavaScript whose exports we can’t analyze, probably because they have some kind of UMD wrapper. When that happens, we try guesses. The quality/presence of guesses is impacted by:
@types
package, if present, matches that of its JS counterpartimport { add } from "lodash"
currently returns three results; two refer to the correct function, but one is a false positive (SetCache.prototype.add
):Function parameters, and properties accessed on them, are unlikely to return any results, because definition information doesn’t tell us every caller of the function—we only know where the parameter itself is declared, and what type it expects. The values passed into it could come from anywhere, even if in practice they have a single definition provided by a library:
FAQ and Q that should be FA but aren’t
Can you please make this the default / give me an option to make this the default for Go To Definition / ⌘-click?
Not right now, but comment / 👍 an existing relevant comment if you feel strongly about this and tell me a bit about why.
Can I make a keyboard shortcut for this?
Yes. Command palette → Preferences: Open Keyboard Shortcuts (JSON)
What’s the difference between this and Go To Implementations?
Go To Implementations is an interesting algorithm that does a lot of different things and is a bit hard to explain holistically. The two commands are similar in that they both avoid returning ambient results. However, Go To Implementations uses Find All References under the hood to (among other things) search for values that satisfy some type. Go To Source Definition mostly uses Go To Definition under the hood, which traces variables/properties to their declarations and through imports/exports. It does not try to find the origin of every possible value that might come to inhabit that variable/property. The thing that makes Go To Source Definition different from every other command is that if Go To Definition returns only results in
.d.ts
files, it will try again with a different module resolver that completely ignores.d.ts
files, allowing it to find JS files that are otherwise “shadowed” by.d.ts
files in your actual compilation.I’m a DefinitelyTyped maintainer. What can I do to increase the chances Go To Source Definition works on exports from my definitions?
Ensure the directory structure of your definitions is identical to that of the JS library you’re typing (at least as far as the public API goes). This means don’t move types out of
index.d.ts
into ahelpers.d.ts
just for code organization—helpers.d.ts
should only exist if it types ahelpers.js
file. Conversely, if the JS library is broken up into many files that all get re-exported by the index/main file, declare your types in these files and re-export them too. (This is the correct way to author DT libraries anyway; Go To Source Definition compatibility is just a secondary benefit.)I’m a library author who ships my own types. What can I do to make Go To Source Definition work on exports from my library?
Consider if it’s appropriate to ship your TypeScript sources to npm. If you do, make sure you compile with
"declarationMap": true
and ship your.d.ts.map
files to npm too. When you do that, Go To Definition will already navigate to your.ts
sources, making Go To Source Definition unnecessary (it simply executes Go To Definition when this happens).The text was updated successfully, but these errors were encountered: