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

Avoid auto-importing from barrel re-exporting index files that are likely to make an import cycle #47516

Merged
merged 3 commits into from
Feb 10, 2022

Conversation

andrewbranch
Copy link
Member

Fixes #45953
Slightly better version of #47432

Essentially the same logic as #47432, but does it by file location rather than computed module specifier to allow for non-relative specifier preferences with baseUrl and paths and symlinks.

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Jan 19, 2022
@fatcerberus
Copy link

If this is specifically just de-prioritizing stuff like ../.. as suggested in the description of #47432, won't the problem show up again with e.g. nodenext when the offending import would be instead ../../index.js?

@andrewbranch
Copy link
Member Author

This is not looking for computed module specifiers, but it does treat files named "index" specially. See the conversation near the end of the linked issue. Only --moduleResolution node gets this behavior because in other modes, I don’t think it makes sense to make assumptions about files named "index." A module specifier like ../../index.js generally doesn’t have this problem anyway, because it has the same number of directory separators as ../../foo.js, so the latter will be sorted first anyway. But yes, this does not universally solve the problem of creating an import cycle with auto-imports, but it should make it less likely for common scenarios.

@fatcerberus
Copy link

fatcerberus commented Jan 20, 2022

I don’t think it makes sense to make assumptions about files named "index." [in modes other than --moduleResolution node]

That's fair, you're not as likely to have index.js be a barrel if you have to name it explicitly, and if there is a barrel somewhere in there it'd probably have a more descriptive name. That being said, it might be an issue for CommonJS projects migrating to ESM since the index.js will exist for historical reasons. ../../foo.js may be sorted first, but ../../quux.js would be sorted after (assuming alphabetical sorting).

@andrewbranch
Copy link
Member Author

It’s not alphabetical sorting, it’s program order, so re-exports come second unless there’s a reason to prefer them (i.e. fewer directory separators).

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Basic idea sounds right to me. The code looks good as far as I am familiar with it -- but I have to relearn it a bit every time.

function sortFixes(fixes: readonly ImportFixWithModuleSpecifier[], sourceFile: SourceFile, program: Program, packageJsonImportFilter: PackageJsonImportFilter): readonly ImportFixWithModuleSpecifier[] {
return sort(fixes, (a, b) => compareValues(a.kind, b.kind) || compareModuleSpecifiers(a, b, sourceFile, program, packageJsonImportFilter.allowsImportingSpecifier));
function sortFixes(fixes: readonly ImportFixWithModuleSpecifier[], sourceFile: SourceFile, program: Program, packageJsonImportFilter: PackageJsonImportFilter, host: LanguageServiceHost): readonly ImportFixWithModuleSpecifier[] {
const _toPath = (fileName: string) => toPath(fileName, host.getCurrentDirectory(), hostGetCanonicalFileName(host));
Copy link
Member

Choose a reason for hiding this comment

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

toPath is used as a way to get the current directory and getCanonicalFileName from the provided host as far as I can tell. Is that basically right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. I think the current directory is never used in practice in this context. What it’s going to do is lowercase stuff.

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
Archived in project
Development

Successfully merging this pull request may close these issues.

Auto-import prefers parent index.ts which leads to circular reference
4 participants