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

Sort import completions by distance from current module #41083

Closed
dhoulb opened this issue Oct 13, 2020 · 9 comments · Fixed by #46703
Closed

Sort import completions by distance from current module #41083

dhoulb opened this issue Oct 13, 2020 · 9 comments · Fixed by #46703
Assignees
Labels
Domain: Auto-import Experience Enhancement Noncontroversial enhancements Fix Available A PR has been opened for this issue Rescheduled This issue was previously scheduled to an earlier milestone Suggestion An idea for TypeScript

Comments

@dhoulb
Copy link

dhoulb commented Oct 13, 2020

The auto-import feature is amazing. Something I come across daily though is the import I want is nearly always the second or third choice.

In the screenshots below VS Code suggests to import db from ../../admin/helpers/db before ../helpers/db

I would propose that the ordering is sorted by distance from the current file. The following ESLint rule (that I use in most of my projects) does this correctly and presumably has code that can be reused: eslint-plugin-import/order (well, except we want the inverse of this — shortest distance first).

Screenshot 2020-10-13 at 14 14 46

Screenshot 2020-10-13 at 14 14 39

@dhoulb
Copy link
Author

dhoulb commented Oct 13, 2020

Here is an additional screenshot showing the problem suggestions too:

Screenshot 2020-10-13 at 14 27 36

@mjbvz mjbvz transferred this issue from microsoft/vscode Oct 13, 2020
@mjbvz mjbvz removed their assignment Oct 13, 2020
@DanielRosenwasser DanielRosenwasser added Domain: Auto-import Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Help Wanted You can do this Suggestion An idea for TypeScript labels Oct 14, 2020
@DanielRosenwasser DanielRosenwasser changed the title Auto-import sorting potential imports by distance Sort import completions by distance from current module Oct 14, 2020
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 4.2.0 milestone Oct 14, 2020
@andrewbranch
Copy link
Member

So, we already do this sorting between different paths for the same symbol. This example must be three distinct dbs that just happen to share the same name, and it looks like they probably just come in the program order. The design question that remains here is what the sorting should be in a combination of these two scenarios:

Import 'db' from module "../foo" ----------------
Import 'db' from module "../foo/helpers"         | -- (aliases for same symbol)
Import 'db' from module "../foo/helpers/index" --
Import 'db' from module "../bar" ----------------
Import 'db' from module "../bar/helpers"         | -- (aliases for same symbol)
Import 'db' from module "../bar/helpers/index" --

I would think we still want to keep aliases for the same symbol grouped together. So then, sort the groups by the closeness of their closest option?

@dhoulb
Copy link
Author

dhoulb commented Oct 28, 2020

This example must be three distinct dbs that just happen to share the same name

That's correct, they're all different symbols with the same name. For context the project is a monorepo that contains multiple different apps (admin website, production app, backend serverless functions). Each has its own database code but they all have similar file layout. When I'm working on a file in the admin website I would never want the other db values.

I use the ESLint import/no-restricted-paths rule to catch when I incorrectly import across an invalid boundary, but it'd be lovely if the auto import worked too.

Regarding your design question: I think the distance sorting is the most likely requirement in nearly every case. The fact the developer has gone to the trouble of re-exporting higher up also implies they don't want highly specific imports. So I think ../foo and ../bar should be the top two suggestions, then ../foo/helpers/ and ../bar/helpers as they're further away and deeper (you're also intruding on the internal structure of the foo directory), and the two index options last (these are way too specific and unlikely to ever be wanted).

@andrewbranch
Copy link
Member

This, like #31658 and #42005, is blocked on us not resolving full module specifiers for auto-imports until a user focuses a completion item. We can’t sort by paths because we don’t know what the paths will be when we render the list. In the simplest cases, it’s a simple relative path from the importing file to the imported one, but that can change if any of the following get involved:

  • node_modules
  • files named index
  • symlinks
  • paths
  • project reference redirects

We could make a rough guess without considering these things, since as far as I can tell, the current order is basically arbitrary (based on program order). I don’t have a strong opinion on whether or not that’s worth it. My hunch is that guessing would produce correct/desirable results far more often than not, but that same-named distinctly-declared exports are rare enough that almost nobody would notice.

The same constraint doesn’t apply to codefixes—we have the full module specifier of every suggestion at the same time, so we can sort those by whatever we want.

@dhoulb
Copy link
Author

dhoulb commented Feb 2, 2021

@andrewbranch Thanks for the investigation! You're right even sorting the relative paths seems like it would solve my specific use case.

I agree it's a niche request — probably mainly useful to people who have projects with folders/structures that follow a single repetitive template, (i.e. exporting a main() from different scripts or sub modules).

@andrewbranch
Copy link
Member

Because I find #31658 and #42005 super compelling, I plan to investigate whether we can resolve module specifiers up front in the future with the help of some additional perf investments. Because of that, I think I don’t want to implement a best-guess sorting for completions before that. But I’ll look into sorting the codefixes today.

@andrewbranch andrewbranch removed the Help Wanted You can do this label Feb 2, 2021
@andrewbranch andrewbranch added Experimentation Needed Someone needs to try this out to see what happens Rescheduled This issue was previously scheduled to an earlier milestone Needs Investigation This issue needs a team member to investigate its status. and removed Experimentation Needed Someone needs to try this out to see what happens labels Feb 2, 2021
@heroboy
Copy link

heroboy commented Mar 1, 2021

Here is my suggestion of the sort criteria

  1. Current and sub paths sorted by depth from current path.
  2. Rest ones sorted by depth from the root.
  3. node_modules

Here is my explain:
Why node_modules is last?
Because normally I shouldn't define symbol conflit with that in node_modules, but I do, I intend to override it.

Why the order of 1 and 2?
Because a file works more closly with sub folder files. Symbols from sub folders are just like internal/private symbol. Every files/modules has their internal symbol. So show current file's internal symbol in priority.
And some base and utility symbols are not define very far from root, so show them in second.

@bpasero
Copy link
Member

bpasero commented Apr 2, 2021

I second this and often end up getting frustrated in VSCode when imports are suggested like so:

To give more context: here I edit core VSCode code but get suggestions that are a) from a d.ts file that is not even close to the one I am in (vscode) or b) is more distant in general.

I think the rules should be to prefer imports from modules by distance but also to prefer non d.ts / node_modules imports if possible even if the distance ends up to be greater.

@andrewbranch
Copy link
Member

This is now more or less possible since #44713 was merged, but takes quite a bit more work on top of that to implement. I’ll plan on taking a look for 4.5.

@andrewbranch andrewbranch added Experience Enhancement Noncontroversial enhancements and removed Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Needs Investigation This issue needs a team member to investigate its status. labels Jul 6, 2021
@typescript-bot typescript-bot added the Fix Available A PR has been opened for this issue label Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: Auto-import Experience Enhancement Noncontroversial enhancements Fix Available A PR has been opened for this issue Rescheduled This issue was previously scheduled to an earlier milestone Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants