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

[gordonmleigh]

Bug Report

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

1. I write a file a/Foo.ts which contains export const Foo = 'Foo'
2. I write a root index.ts which contains export * from './a/Foo'
3. I write a file b/Bar.ts and attempt to auto-import Foo
4. 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.

[JonWallsten]

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

It broke in this release: It broke in [email protected]

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.

[rezonant]

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';

[rezonant]

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.

[inca]

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 🤞

[ziofat]

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.

[inca]

@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).

[rezonant]

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?

[ziofat]

@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?

[rezonant]

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.

[andrewbranch]

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.

[ziofat]

@andrewbranch
Just look at this file in rush.js. It is really common to export most classes and types in entry file for npm packages.

[gordonmleigh]

I agree with this. Current best-practice seems to me to result in never using the barrel file with a relative path. The barrel file is there to provide a defined interface for the package, and therefore will normally be accessed via package imports. Files within the package will import from the files which contain the actual definitions, never re-exports.

[JonWallsten]

In our case ignoring re-exports, or barrel files if it's possible to identify them, and always use the relative path to the file would be the best option. As other have state barrel files are usually used outside of the application. I undestand some people might still want to use them, so I guess an option could be a nice way to accommodate everyone. If I have to choose between always or never use a barrel file inside my own package, it choose never everyday of the week. It's really not needed anymore since VSCode IntelliSense has improved over the years. And now when it also refactor paths when renaming and moving files around I don't see a use for internal barrel files.

[andrewbranch]

Does this not break down when your app gets big enough? I used to work on a web app frontend at Microsoft where we had 5 or so individual teams working on sections of the app, all consuming an internal library of UI components that were part of the same app (there was no reason to separate these into separate packages, it was just a big app with code splitting). We defined Webpack aliases and TypeScript paths and exported every component from a barrel so that the 5 feature teams could import components like import { Card, Table, List } from "@common/ui". Not a separate package, just a convenience over "../../../../common/ui". Each component was in its own folder under ui, and by convention had a main implementation file and its own little barrel re-export, since some components had variants or utils in separate files. Under the rules you’re proposing, that would become

import { Card } from "@common/ui/card/Card";
import { Table } from "@common/ui/table/Table";
import { List } from "@common/ui/list/List";

That’s preferable??

I suspect the gold standard heuristic would be “do not import from a re-export that also imports me (the importing file).” So in a conventional structure, you would be fine importing from "../foo[/index.js]" rather than "../foo/export.js", because "../foo/index.js" only imports (and re-exports) "../foo/*", which does not include the sibling folder of foo which we’re currently in. On the other hand, you would not be ok to import from "..[/index.js]" because we are inside the same directory as the re-exporting file, therefore we might also be imported/re-exported by it. This is captured by my rule of “only dots and slashes,” but only if you use relative paths, CommonJS resolution leaving off index files, and your re-exporting files are named index. It turns out that accurately capturing the information that was intended to proxy, “does this file also ultimately import me,” is difficult and expensive to capture, so some assumptions and heuristics will probably have to be used.

[rezonant]

I definitely use barrels within my applications for modules from outside the module. Inside the module I don't use the barrel for that module for obvious reasons

[andrewbranch]

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

[andrewbranch]

I think I’m going to go with “don’t import from a re-exporting file that is a direct child of any ancestor directory of the importing file.” That should cover 100% of cases that #47432 did, plus scenarios with non-relative paths, ESM resolution, and barrels not named “index.” It’s probably not perfect, but it’s fast.

[inca]

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

schema/
    index.ts        // export * from './User'; export * from './Group';
    User.ts         // export class User {}
    Group.ts        // export class Group {}
services/
    index.ts
    AuthService.ts
index.ts            // export * from './schema'; export * from './services';

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

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).

I'm assuming the #47432 will work just fine for covering these.

[andrewbranch]

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.

1. schema/User.ts is not a re-export
2. schema/index.ts’s directory is schema/ which is an ancestor of schema/Group.ts: don't use this
3. 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).

1. schema/User.ts is not a re-export
2. schema/index.ts’s directory is schema/ which is not an ancestor of services/AuthService.ts: this is fine
3. 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.

[rezonant]

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

@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.

[rezonant]

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

I think this satisfies all my use cases 👍

[andrewbranch]

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.

[andrewbranch]

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

[rezonant]

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.

[ivancuric]

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.

[hymair]

This is definitely not resolved. Running into the same behaviour as @ivancuric

@andrewbranch can this be re-opened?

[RyanCavanaugh]

Please open a new issue with concrete repro steps

[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!

[AlokTakshak]

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!

@andrewbranch do we have any plan to backport this to the earlier versions?

[andrewbranch]

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