Don’t auto-import paths that are in a referenced project’s outDir

[andrewbranch]

Fixes #38856

We started getting auto-imports from referenced project outDirs due to #37482. We do potentially need to look in outDir because the result could be the typings or main entry of the project’s package.json (which would be referenced in symlinked monorepo setups), in which case the resulting module specifier would be the bare non-relative package name. But if that file does not match the typings or main entry (or cannot be referenced through node_modules), we don’t want to use that file as a possible path to import, and we certainly don’t want it to crowd out the non-redirect source file.

[andrewbranch]

The failing unit tests make me want to refactor this further. We throw away too much info in getAllModulePaths and forEachFileNameOfModule. Converting this to draft, don’t bother reviewing yet.

[sheetalkamat]

But is this really intended behaviour? In real scenario (non editing) the package will still use outDir and not source.. And normal build will use .d.ts files instead of source files(source files from project reference are used only to give better editing experience) so why is ref/src/a better than ref/dist/a

[andrewbranch]

Really, it depends on the referenced project’s rootDir and what folder they plan to publish to a package registry. We can’t know the latter, so the truth is that we don’t know for sure what path will be correct at runtime. My own conventions would lead me never to create a system where I explicitly import from dist, but probably not everyone shares those conventions. We could offer both paths as suggestions, but that could get frustrating, as people will consistently want only one of the two suggestions, and the presence of the other will slow them down and feel like a bug.

[sheetalkamat]

Well from package point that may be true but from project references stand point the output is what matters (since you dont want to include the source file in your build) and hence i am not sure offering "src" vs "dist" is improvement.

[andrewbranch]

That depends on the relationship between the outDir of the importing file and the outDir of the imported file. Consider this non-monorepo project references scenario:

src/
  a/
    tsconfig.json - outDir: ../../out/a
    index.ts
  b/
    tsconfig.json - outDir: ../../out/b
    index.ts

out/
  a/
    index.js
    index.d.ts
  b/
    index.js
    index.d.ts

Because the outDirs of the two projects have the same relationship as the rootDirs, it’s safe and conventional to write imports from a to b as import { B } from "../b";. But as of #37482, you get two suggestions:

I can’t see any reason why you would ever want the latter. It’s not wrong on disk now, but what happens if you use out as the context for a Docker build, copying files to a container image? That image will be broken unless the working directory on the container is also named out. These module specifiers aren’t portable.

[bradennapier]

Running into this as well - https://github.com/microsoft/vscode/issues/103087

https://github.com/bradennapier/typescript-auto-import-bug

auto import completely broken

[andrewbranch]

This is superseded by #40253

[dko-slapdash]

@andrewbranch So this is like a breaking change in comparison to v3.9 if I understand correctly, right?

In a monorepo setup (e.g. lerna & yarn workspaces), e.g. packages/shared is auto-symlinked to node_modules/shared. So in src+dist configuration, it's actually desired that IntelliSense suggests importing from shared/dist/ and not from shared/src/ - otherwise the paths won't just work at runtime.

I posted some details here: #34677 (comment)

The screenshot with an illustration on when it's desired to import from outDir packages/shared/dist/ is following:

[andrewbranch]

This didn’t get merged, so I would say it’s not a breaking change?

[dko-slapdash]

This didn’t get merged, so I would say it’s not a breaking change?

This effect is reproducible on v4.x (e.g. on 4.0, on 4.0-beta etc.) too. So it's here for quite some time.

[andrewbranch]

The effect of this PR is not reproducible in any TypeScript version because it is not part of TypeScript. It was an experiment that failed and did not get merged. You may need to set up paths to ensure the auto-import behavior you’re looking for. Please read the description of #40253 (beginning at the “Background” heading) for more info. Feel free to ask follow-up questions on that PR, since that one was actually merged.