Skip to content

Go to definition shows Node require statement as definition #39895

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

Closed
gorgos opened this issue Aug 4, 2020 · 8 comments · Fixed by #40835
Closed

Go to definition shows Node require statement as definition #39895

gorgos opened this issue Aug 4, 2020 · 8 comments · Fixed by #40835
Assignees
Labels
Bug A bug in TypeScript Domain: Symbol Navigation Relates to go-to-definition, find-all-references, highlighting/occurrences. Fix Available A PR has been opened for this issue

Comments

@gorgos
Copy link

gorgos commented Aug 4, 2020

I'm simply doing

const { calcMargin } = require('../../order-utils');
...
calcMargin()

and the calcMargin call shows two definitions: 1. the require line and 2. the actual function definition that I want to go to.

vs-goto-small

Version: 1.47.3
Commit: 91899dcef7b8110878ea59626991a18c8a6a1b3e
Date: 2020-07-23T13:08:29.692Z (1 wk ago)
Electron: 7.3.2
Chrome: 78.0.3904.130
Node.js: 12.8.1
V8: 7.8.279.23-electron.0
OS: Darwin x64 19.6.0

Related but not identical to #37816.

@DanielRosenwasser DanielRosenwasser added Bug A bug in TypeScript Domain: Symbol Navigation Relates to go-to-definition, find-all-references, highlighting/occurrences. labels Aug 5, 2020
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 4.1.0 milestone Aug 5, 2020
@andrewbranch
Copy link
Member

@sandersn, this could be impacted by your explorations into making CommonJS requires actual alias symbols. Is that still something you’re looking at?

@sandersn
Copy link
Member

sandersn commented Aug 5, 2020

Yes, I've been distracted by other JS prototypes for 4.1, but I'll get back to it pretty soon. I'll add this bug to the list of ones to check on.

@sandersn
Copy link
Member

Duplicate of #25533; this bug has a plainer explanation of the problem, and that one has a ready-made repro. #39770 is definitely a pre-requisite for fixing this, and I hope to get this fix in at the same time.

@andrewbranch andrewbranch added Duplicate An existing issue was already created and removed Bug A bug in TypeScript Domain: Symbol Navigation Relates to go-to-definition, find-all-references, highlighting/occurrences. labels Aug 11, 2020
@sandersn sandersn added Domain: Symbol Navigation Relates to go-to-definition, find-all-references, highlighting/occurrences. Bug A bug in TypeScript and removed Duplicate An existing issue was already created labels Aug 12, 2020
@sandersn
Copy link
Member

Well, on digging into the code, I found two things:

  1. We actually have quite different code for goto-def.
  2. For goto-def to work the same way as with ES imports, both const = require AND module.exports= need to create aliases. Alias for commonjs require in JS #39770 only does the former. (Alternatively, we could add more ad-hoc resolution code in gotoDefinition.ts.)

#39770 does fix find-all-refs and improves goto-def -- it jumps to the module.exports -- but it still doesn't jump all the way to the original declaration like ES exports to. So, although the OP's screenshot is a dupe of #25533 for find-all-refs, the title requests a goto-def fix. Let's use this bug to track that now.

@andrewbranch
Copy link
Member

both const = require AND module.exports= need to create aliases.... (Alternatively, we could add more ad-hoc resolution code in gotoDefinition.ts.)

@sandersn do you have an opinion on which of these is a better solution? I.e., is making module.exports aliases valuable beyond the benefits to go-to-definition? I have a feeling that might be a bit risky for this post-beta period, but is it something we should be aiming for otherwise?

@sandersn
Copy link
Member

sandersn commented Sep 24, 2020

As of 4.0, both module.exports.x= and module.exports= create an alias for entity names and class expressions, although not for other expressions. So the rest of this fix is likely in gotoDefinition.ts -- but it's hopefully less ad-hoc than before.

Changes in goto-def are less core to the compiler, but might be more complex. It's hard to guess whether this should ship in the RC without making the fix first. If turns out to be simple, I think it would be fine.

@andrewbranch
Copy link
Member

I actually don’t know if this need be specific to module.exports or even to JavaScript. I kind of feel like we should always just jump past shorthand property assignments... maybe? They feel very alias-y, not very declaration-y.

@andrewbranch
Copy link
Member

There’s really not a lot of user feedback on this, and offering too-granular definitions is generally better than making a non-obvious leap past the definition the user was interested in (skipping past an uninteresting declaration is usually as simple as an additional ⌘-click). So, I’ll limit this to skipping past the shorthand property assignments in module.exports = { one, two, three, ... } as I feel confident from personal experience that those are a nuisance when they show up in go-to-definition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Domain: Symbol Navigation Relates to go-to-definition, find-all-references, highlighting/occurrences. Fix Available A PR has been opened for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants