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

Bug Report

This appears to be recently changed behaviour. I have a package with an index.ts at the root:

I write a file a/Foo.ts which contains export const Foo = 'Foo'
I write a root index.ts which contains export * from './a/Foo'
I write a file b/Bar.ts and attempt to auto-import Foo
The import is written as import { Foo } from '..'.

I think that importing from an index directly above the file like this is highly likely to lead to a circular reference.

🔎 Search Terms

auto-import auto-import relative

🕗 Version & Regression Information

This changed between versions 4.3.5 and 4.4.2

⏯ Playground Link

See simple repro.

💻 Code

// index.ts
export * from './a/Foo';

// a/Foo.ts
export const Foo = 'Foo';

// b/Bar.ts
import { Foo } from '..'; // this is the resulting import

const Bar = Foo; // try to auto-import this

🙁 Actual behavior

Imports from parent index.ts. This file is highly likely to also import the importing file, causing a circular reference.

🙂 Expected behavior

Import from './a/Foo' instead. Or, put another way, prefer not to import from a path which has only one or more .. in it.

Since my branch was auto closed I'll move some information to this one.

It broke in this release: It broke in typescript@4.4.0-dev.20210626

Most likely in this commit:
Resolve module specifiers for auto imports in completion list 328e888

My thoughts:
I appreciate that it's hard to figure out what to do, because it's impossible to know the users intentions. One solution could be to either have an option for or always also show the "source file path" when it's reexported. I think it was like that before:

It would at least give the user a choice, but I agree it's not optimal since it creates a mess in the suggestion lists and I guess it was why this was "fixed"?

Another solution could be to have an option for or always ignoring re-exports from the entry point or all index.ts (or all re-exports and always resolve the source file).
That entry point at least is most likely for consumers of the package, and not to be used internally.
I'm conflicted about other nested index.ts files in the project. It could be a case where you have a utils/index.ts to expose utils for your own needs, but auto imports removes the need for that. I guess it also messes with tree shaking.
But if it's about all or nothing, I would be glad to have the option to always resolve the source file when importing files locally in a package.

Can exhibit as well with a '.' import, seems to just depend on how far up the symbol is exported.

My example from #46817 was:

Given a folder with:

// index.ts
export * from './a';

// a.ts
export class A {}

// b.ts
export class B extends A {}

...an attempt to use auto-import to resolve the reference to A in b.ts will insert:

import { A } from '.';

...instead of the preferred (and the one provided by previous versions of VS Code):

import { A } from './a';

Duplicate of #45953. We’ll probably drop paths that resolve to nothing but dots and slashes as that often creates an import cycle between a barrel re-export and the file that’s importing it (which may not be a problem at runtime, but can be if the modules have side effects, and I’ve just never seen anyone use import paths like that on purpose).
#47040 (comment)

It is a huge relief to hear that there is an intent to fix here, auto-importing is effectively useless in VS Code right now due to this issue :-( Have been working around it but if I don't notice it happens it inevitably causes problems at runtime, usually due to decorator references or runtime type metadata.

Sorry to pitch in without any additional info (I think the problem description is spot on), I would only like to note on the severity of this: right now VS Code + TypeScript >= 4.4.0 is effectively unusable if the modules are structured using re-exports. In the codebases that adopt this pattern at scale, all auto-imports must be essentially re-written manually, which is a huge bummer. The only sensible alternative at the moment is downgrading to TypeScript < 4.4.0 which can also tricky because the codebase may already have adopted the 4.4's breaking changes.

Hoping this will get resolved soon 🤞

I noticed there is a fix and it prevents '../' like paths. Good enough for relative path users.
How about absolute path? 'src' is kind of the same thing that bothers me.

ziofat (@ziofat)

ziofat (@ziofat) Not sure I understand that right, but how can absolute path consist of only . and / characters? Any examples? (the fix specifically removes candidates such as '.', '..', '../..' and the likes — which I can confirm to be the only problematic cases I've seen — and everything else is left intact).

I think the point is that when auto importing files in the root (src for instance), "src" is the shortest path, but the dev may wish to use a relative import instead of "src". Is that right?

Boris Okunskiy (@inca)

Boris Okunskiy (@inca) Sorry for my poor expression.
What I mean is, I have a setting in VSCode which indicate to import modules from absolute path:

And when I try to import some module which is re-exported it always give me src:

If I set the setting to 'relative' it will give me ../../ which is what this issue about.

In this example, src and ../../ are basicly same because Typescript only emits the 'shortest' import path. Filtering dots and slashs does not resolve this issue, it is just a workaround.

My project is enforced to use absolute path importing because it provides more context about project structure to improve readability. And we have to remove all contents in src/index.ts to make auto import work properly when we are developing.

In my opinion this issue should stay open for better solution. For example, providing a 'deepest' option to always ignore re-export?

I do think the "shortest path" model is too simplistic. I thought previously it was using the shortest path below the current directory so as to avoid loops while still taking advantage of barreled modules appropriately, was this not the case before?

So for instance if I was in foo/bar.ts and I imported Something from foo/baz/something.ts and it was exported in foo/baz/index.ts, I'd get "./baz" (with the relative option), but if I was in foo/baz/other.ts and I imported that same symbol I would get "./something".

Under the shorter rule wouldn't it still prefer "../baz" since that is shorter than "./something"? Or is this rule based on number of components rather than string length?

I was going to test this behavior with the new fix using Insiders to better understand but for some reason auto-importing didn't work at all in Code Insiders (no fix recommendation at all). Fresh Insiders installation with no extensions.

EDIT: I got autocomplete to work in Insiders.... it requires you to have the files open in the editor in order for the suggestion to be available (and if you close the file providing the symbol, it loses the suggestion). Seems like there's bugs to fix there unrelated to this. I was able to check the current behavior but it looks like the "no dots and slashes" change is not in Insiders yet as it still suggests "." pretty readily.

For example, providing a 'deepest' option to always ignore re-export?

🤔 Why write a re-export if you never want to use it? The behavior you’re asking for will happen if you simply delete your barrel re-export.

don’t import from a re-exporting file that is a direct child of any ancestor directory of the importing file

In schema/Group.ts importing User should be import { User } from './User' and not import { User } from '.'.

Our options are schema/User.ts, schema/index.ts, and index.ts.

schema/User.ts is not a re-export
schema/index.ts’s directory is schema/ which is an ancestor of schema/Group.ts: don't use this
index.ts’s directory is [implied base dir] which is an ancestor of schema/Group.ts: don't use this

(2) and (3) are deprioritized.

In AuthService.ts importing User should be import { User } from '../schema' and not import { User } from '..' (also not import { User } from '../schema/User' because this somewhat ignores the logical "module" boundaries).

schema/User.ts is not a re-export
schema/index.ts’s directory is schema/ which is not an ancestor of services/AuthService.ts: this is fine
index.ts’s directory is [implied base dir] which is an ancestor of services/AuthService.ts: don't use this

(3) is deprioritized, leaving (1) and (2) to move on to other sorting mechanisms. If you’re using CommonJS resolution leaving off index filenames, (2) will win as "../schema" by merit of having fewer directory separators.

William Lahti (@rezonant)

William Lahti (@rezonant) can you rephrase or expound on that? A module is any file with a top-level import or export.

Andrew Branch (@andrewbranch) I meant it in the sense of "feature module", for instance if I had a folder "accounts" that had all my accounts related classes, models, etc in it, I would stick an index.ts in accounts folder and expect to import it via the barrel from outside of the module. Within the module I use relative imports, so for example in accounts/user.ts perhaps it would import Profile from "./profile".

If the accounts module needed to use another sibling module, say for instance next to "accounts" I've got "util" or so, I would import from that util module from within an accounts file as "../util".

In some very large apps I make "@" a top level paths entry and set VS to absolute imports such that I would get `import { ... } from "@/util" or so, but I haven't checked if the current revision of auto import gets this right off the bat or not.

The obvious benefit of both barrel imports internally in an app as well as absolute imports via tsconfig paths is to aid in refactoring to reduce the amount of import fixups required when things change or are restructured, as well as reducing the number of imports needed when importing many classes from one "feature module". Another benefit is when the internal modules are expected to eventually spin out into their own NPM modules. Restructuring module directory structures to ensure non-cyclical dependencies is extremely important when using decorators unfortunately, since the decorator format does not use the accessor pattern (ie () => SomeType) and cannot be changed at this point. When decorators hits stage 3 the hope is that this will be opened back up for breaking changes.

I think I can provide an example. Let's assume the following directory structure:

I think this satisfies all my use cases 👍

Counterexample against my own proposed heuristic: suppose you have a utils.ts that exports some locally declared functions and also re-exports some things from libraries or far-away modules for convenience:

// project/ui/utils.ts
export function sum(a, b) { return a + b }
export { tryParseJson } from "../api/long/path/internal/utils";

// project/ui/pages/home.ts
tryParseJson/*|*/

Here, by my rule, we would avoid importing from "./utils" because it is directly contained in an ancestor directory of the importing file. But "./utils" poses no circular import threat. It specifically re-exported the far-away thing so as to provide a closer and more convenient import path for that util to its neighbors.

One helpful hypothesis might be that the files we want to avoid importing from are

either an entrypoint in the package.json file (main or in exports), or
named index and always in CommonJS resolution mode

Yes I think treating index specially when moduleResolution: "node" is selected works for me at least (with the ancestor rules you are defining above).

I think a more general solution would require doing more complex import analysis -- ie if the import candidate "depends" on the current file via zero or more indirect import paths, then deprioritize it. Probably would be expensive.

I'm still getting barrel files as the top autocomplete. My project is set up as:

core/		// provides core functionality
ui/			// builds upon core functionality
index.ts	// exports both from ui and core, exposed in package.json

Files from ui/* prefer re-exports from index.ts, no matter how it's named, or what the moduleResolution is set up as.

Ivan Čurić (@ivancuric)

This is definitely not resolved. Running into the same behaviour as Ivan Čurić (@ivancuric)

Andrew Branch (@andrewbranch) can this be re-opened?

Please open a new issue with concrete repro steps

or what the moduleResolution is set up as

This was exactly what mattered, in fact. The original fix was incorrectly limited to --moduleResolution node. Fixed at #57342. Thanks!

Andrew Branch (@andrewbranch)

or what the moduleResolution is set up as

This was exactly what mattered, in fact. The original fix was incorrectly limited to --moduleResolution node. Fixed at #57342. Thanks!

Andrew Branch (@andrewbranch) do we have any plan to backport this to the earlier versions?

Nope. We almost never patch anything but the current minor release, and then only for critical regressions.

Navigation Menu

Repository navigation

Auto-import prefers parent index.ts which leads to circular reference #45953

Bug Report

🔎 Search Terms

🕗 Version & Regression Information

⏯ Playground Link

💻 Code

🙁 Actual behavior

🙂 Expected behavior

9 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Auto-import prefers parent index.ts which leads to circular reference #45953

Description

Bug Report

🔎 Search Terms

🕗 Version & Regression Information

⏯ Playground Link

💻 Code

🙁 Actual behavior

🙂 Expected behavior

Activity

JonWallsten commented on Nov 14, 2021

rezonant commented on Nov 18, 2021

rezonant commented on Dec 30, 2021

inca commented on Jan 13, 2022

ziofat commented on Jan 16, 2022

inca commented on Jan 16, 2022

rezonant commented on Jan 16, 2022

ziofat commented on Jan 17, 2022

rezonant commented on Jan 17, 2022

andrewbranch commented on Jan 18, 2022

9 remaining items

andrewbranch commented on Jan 19, 2022

rezonant commented on Jan 19, 2022

rezonant commented on Jan 19, 2022

andrewbranch commented on Jan 19, 2022

andrewbranch commented on Jan 19, 2022

rezonant commented on Jan 19, 2022

ivancuric commented on Oct 17, 2023

hymair commented on Feb 8, 2024

RyanCavanaugh commented on Feb 8, 2024

andrewbranch commented on Feb 8, 2024

AlokTakshak commented on Mar 27, 2024

andrewbranch commented on Mar 27, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions