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

Respect importModuleSpecifierPreference in sort order between fixes for the same symbol from different files #58597

Merged
merged 4 commits into from
May 21, 2024

Conversation

andrewbranch
Copy link
Member

Fixes #55863

There were two bugs in play:

  1. importModuleSpecifierPreference was only being considered in how to generate module specifiers between a given pair of files. But when computing auto-imports, there may be multiple files a missing symbol can be imported from, so the module specifier generation code is called multiple times, and multiple fixes are generated. Those fixes are then sorted, but the sort logic didn’t consider importModuleSpecifierPreference. In most cases, all the generated fixes either successfully satisfied the preference or didn’t, but it’s possible, as in the bug repro and test case, that e.g. the symbol being imported is available from one file that can only be reached by relative path and another file that can be reached by a non-relative paths/baseUrl module specifier.
  2. In the import fix sorting, we also sort by whether the module specifier for the fix is a listed package.json dependency whenever that specifier is non-relative. But a non-relative specifier may be a node_modules package, or it may be generated from paths/baseUrl. We were incorrectly checking whether these paths/baseUrl specifiers were package.json dependencies, which would further deprioritize them under relative paths. (Writing this out makes me realize that I need to follow up with a separate test to ensure that package.json imports and self-name imports are not being deprioritized for the same reason.)

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels May 20, 2024
verify.importFixModuleSpecifiers("", ["#is-browser", "./env/browser.js"]);
Copy link
Member Author

@andrewbranch andrewbranch May 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It always should have been in this order due to number of path segments, but #is-browser was being demoted because it was being checked against package.json dependencies, as I mentioned in the PR description.

@andrewbranch andrewbranch merged commit 84ab39d into microsoft:main May 21, 2024
28 checks passed
@andrewbranch andrewbranch deleted the bug/55863 branch May 21, 2024 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto-import not using up TypeScript path alias when using barrel export (index.ts)
4 participants