microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
99.85k stars 12.36k forks source link

Avoid barrel re-exports in auto-imports #51418

Open TheColorRed opened 1 year ago

TheColorRed commented 1 year ago

Bug Report

šŸ”Ž Search Terms

Bug

When using the setting typescript.preferences.autoImportFileExcludePatterns, it works, however, in a mono-repo I cannot import from another package's root without using the full path to the file when excluding index.ts files.

{
  "typescript.preferences.autoImportFileExcludePatterns": [
    "./packages/*/src/index.ts"
  ],
}

I have a workspace package.json that looks like this:

{
  "name": "workspaces",
  "version": "1.0.0",
  "workspaces": [
    "packages/core",
    "packages/a"
  ]
}

In the package.json of packages/core I have this:

{
  "name": "@org/core",
  "main": "./src/index"
}

Then in the packages/a, I try to import a file from packages/core, but autoimport imports something like this even though it is exported through the main file:

import { MyClass } from '@org/core/src/classes/my-class';

The package.json for packages/a

{
  "name": "@org/a",
  "main": "./src/index"
}

The tsconfig.json for both:

{
  "extends": "../tsconfig.base.json",
  "files": [
    "./src/index.ts"
  ]
}

Expected Result

This might be a feature request, but I think it would be great if barrel files could be ignored in the auto-import if within the same module/package, as this can often lead to circular dependencies that are hard to track down.

RyanCavanaugh commented 1 year ago

Can you set up a sample repo, and also clarify the "expected result" ? Are you expecting a different import path to be generated, or not to be shown that import suggestion at all?

TheColorRed commented 1 year ago

@RyanCavanaugh I have created the repo: https://github.com/TheColorRed/vscode-autoImportFileExcludePatterns

Steps:

  1. Clone the repo
  2. npm i
  3. In packages/helper/src/index.ts type readF and select the one that is in @org/core from the Intellisense popup.

You will then notice the auto import imports the full path to the file:

import { readFile } from '@org/core/src/files';
readFile

Now if you turn off the setting typescript.preferences.autoImportFileExcludePatterns in .vscode/settings.json and use Intellisense, the import works the way I intended:

import { readFile } from '@org/core';
readFile

Having this setting works the way I want when referencing a file in the current mono-repo project (aka when @org/core or @org/helper uses one of its own files). But when @org/helper uses a file from @org/core the wrong import is used.

Here is what I am looking for:

@org/core     -> import { readFile } from './files';
@org/helper   -> import { readFile } from '@org/core';

Here is what is happening:

@org/core     -> import { readFile } from './files';
@org/helper   -> import { readFile } from '@org/core/src/files';
ssalbdivad commented 1 year ago

I ran into a similar issue with this. It would be great if there were a way to exclude "index.ts" from being suggested directly while still allowing other specifiers that resolve to a file called "index.ts".

ssalbdivad commented 8 months ago

@andrewbranch Any chance this could fit in with some of the ongoing auto-import improvements?

Would make a huge difference to me as currently I either get imports from my main.ts entrypoints within a single package, causing circularities, or cannot get auto imports from other packages in the monorepo.

andrewbranch commented 8 months ago

This is super working-as-intended, and the desired behavior I think clearly doesnā€™t map onto autoImportFileExcludePatterns, unless you had a multi-root workspace and had different VS Code settings for each TS project, which would let you exclude your own index.ts files in each workspace root without excluding any other packageā€™s index.ts files.

Auto-imports has a preference toward barrel files because they usually indicate the most public export of something and result in the shortest path. I would like to move away from this preference since barrel files often make build tools work poorly, but there needs to be a stronger proposal for how to make this shift, and it needs to be motivated by objective reasons, not code style aesthetics.

In the example project here, if you donā€™t want to import from '@org/core/src/files', then that package should declare an "exports" that prevents such a path from being resolvable. After that, if we had a setting for ā€œavoid barrel re-exports,ā€ it would do the job, because in the local project, you might have many ways to access some export, including a barrel file, but there would only be one way to access everything from '@org/core'.

andrewbranch commented 8 months ago

I feel instinctively opposed to a setting like ā€œavoid barrel re-exports in my own files but not from my dependencies.ā€ There are many reasons barrels are bad; the circularity problem is just one. If auto-imports is making a circularity that could have been detected, or at least looks suspicious, we should try to fix that universally, not behind a setting. I did some work like that in https://github.com/microsoft/TypeScript/pull/47516. But given all the other barrel problems, it begs two questions:

  1. Why would you want to import from barrels in node_modules? Your bundler and other tools are probably going to work better and faster if you use the deepest / most specific imports the package allows.
  2. Why write them in your own code?

To me, this makes it feel like we kind of donā€™t need any additional settings. If you want short import paths from node_modules but you donā€™t want to import from your own barrels, maybe you should stop writing barrels in your code. On the other hand, if we changed settings for everyone to avoid barrels everywhere, and people started complaining about getting unwieldy auto-import paths for their dependencies, then maybe those dependencies should define "exports" that declare what subpaths are allowed.

ssalbdivad commented 8 months ago

@andrewbranch Maybe I should have described my use case more clearly but I don't use barrel files (or at least in the sense I normally hear them described). I only have a single main.ts file for each package which constitutes its public API- there are no nested barrel files I ever want to use internally. Is there an alternative to this for a package that wants to publish a top-level API?

I have a paths config like this that allows me to avoid having to build to see my runtime or type changes reflected e.g. in tests:

        "paths": {
            "arktype": ["./ark/type/main.ts"],
            "@arktype/util": ["./ark/util/main.ts"],
            "@arktype/fs": ["./ark/fs/main.ts"],
            "@arktype/schema": ["./ark/schema/main.ts"],
            "@arktype/attest": ["./ark/attest/main.ts"]
        }

If you have another suggestion as to how to achieve this that doesn't result in the problematic suggestions but does allow importing from a path like @arktype/schema that resolves to the entrypoint of another package in the monorepo, I'd definitely be open to it!

ssalbdivad commented 8 months ago

@andrewbranch Rereading your second comment, I understand more what you're suggesting now, but top-level discoverability for the API of a library feels valuable.

I would definitely not want to give that up just to avoid some obnoxious auto-imports that worst-case scenario I can manually fix. Granted it is grating given how often these kinds of imports occur, but I don't know if I can recall a library that doesn't provide some top-level export and I think there are good reasons for that.

andrewbranch commented 8 months ago

IMO you should never use paths. Use package manager workspaces and set up project references. Thatā€™s enough to not have to build to see type changes in the editorā€”references to project reference output files get transparently redirected to input files in the IDE so you get instant IntelliSense updates across projects. For fast test/dev runtime workflows, I add conditional "exports" on top of this:

{
  "exports": {
    "ts": "./src/main.ts",
    "default": "./dist/main.js"
  }
}

then set your test runner / whatever to use the ts condition.

paths goes through some pretty different auto-import code paths so Iā€™m not sure how it might be affecting your experience.

ssalbdivad commented 8 months ago

@andrewbranch I actually had this kind of setup originally but it felt like a lot of extra ceremony for no extra benefit compared to using paths, at least within my own repo.

I also had the same problem with autoimports with that set up.

ssalbdivad commented 2 months ago

@andrewbranch Any new ideas on this one?

I've been talking with @colinhacks about strategies for developing in a TS monorepo with no build step as documented in his blog post here.

This is one of the biggest downsides to either of these approaches. I don't see any way around having at least one "barrel file" for your API entry point that doesn't involve freezing your file structure in order to have a stable release.

andrewbranch commented 2 months ago

Iā€™m open to trying a preferDeepImports preference that applies universally, no carve-outs for local files vs. node_modules.

ssalbdivad commented 2 months ago

@andrewbranch I'm sure that could be useful in some situations but I expect it would result in imports resolving to arbitrary source files across packages in a setup like the one I'm describing.

I'm far from committed to tsconfig paths if there's a better way to achieve this that avoids the autoimports issue. What I'm really wondering is do you...

  1. Agree a config setup that avoids having to build in dev is a big win?
  2. Know of a better method than those mentioned by Colin to achieve this (specifically tsconfig paths paralleling your package names or customConditions)?
  3. Have any suggestions for how to autoimport from a package entry point when outside the package itself in a setup like this?

My first intuition was that perhaps typescript.preferences.autoImportFileExcludePatterns or another setting could support a pattern that would be checked against the import specifier rather than the resolution, but maybe there is a technical reason that is not possible or desirable?

andrewbranch commented 2 months ago

Iā€™ve been using customConditions in the arethetypeswrong monorepo from the beginning to get no-build updates in the Vite dev server. I just donā€™t agree that configuring that is unreasonably burdensome. The entire JavaScript ecosystem has been given a huge gift in that all major tools adopted conditional package.json "exports" almost to spec. Itā€™s not a perfect system, but itā€™s pretty good, and this is a perfect application for it.

a pattern that would be checked against the import specifier rather than the resolution, but maybe there is a technical reason that is not possible or desirable

No, itā€™s both possible and desirable, just nitpicky design questions remain: #55092

ssalbdivad commented 2 months ago

Yeah customConditions is fine with me. It essentially serves the same purpose in my case and I can see how in other scenarios it would be less likely to cause problems getting out of sync, so I can see why you'd recommend it.

I think for now it would fall victim to the same underlying problem, I will keep an eye on the issue and add some context there.

andrewbranch commented 2 months ago

I think for now it would fall victim to the same underlying problem

If Iā€™m understanding you correctly, it just depends on how you set up the "exports" API. You can choose whether you expose deep imports or not. If you just want to have a single package entrypoint that exports everything, you can do that, and then auto-imports within that package will work normally, importing from local files, and imports outside that package will only ever be able to refer to the top-level package specifier. If you choose to expose deep imports on the package by defining many subpaths or a wildcard subpath, along with "." re-exporting everything, then yes, auto-imports will have to make a decision whether to import from the packageā€™s "." or other subpaths, which would be affected by the preference Iā€™m proposing. But I still assert that if you want ā€œdeep importsā€ in some places, you have reasons to want them everywhere šŸ˜„ Am I missing something? I donā€™t really see what the problem is.

ssalbdivad commented 2 months ago

@andrewbranch What you described does explain how preferDeepImports could solve this problem with either setup. That sounds like it could work perfectly as long as it doesn't try to cross package boundaries directly (which I assume it wouldn't since I don't that occurs currently).

What I meant about the same underlying problem though is that given that such a setting doesn't exist currently, both tsconfig paths and package.json exports would be affected by this issue, correct?

andrewbranch commented 2 months ago

One major difference between exports and paths is that exports block the resolution of paths not explicitly listed, whereas paths is purely additive. So already, you can use customConditions and "exports" to limit how a package can be imported externally, whereas paths does not really afford you that ability.

ssalbdivad commented 2 months ago

@andrewbranch Sure but I'm already using exports for that reason and I mostly just have one entry point per package so this doesn't really impact me.

andrewbranch commented 2 months ago

Then I donā€™t understand what problem youā€™re currently having šŸ˜… The OP complained about auto-imports like '@org/core/src/classes/my-class' which shouldnā€™t be possible if your equivalent of @org/core/package.json has an "exports" (assuming youā€™re using a modern moduleResolution setting)

ssalbdivad commented 2 months ago

I have a package arktype that depends on @arktype/schema. @arktype/schema has a primary API entrypoint at ark/schema/api.ts.

To avoid importing from the API entrypoint from within @arktype/schema, which would often create cyclic imports, I can add the following config:

    "typescript.preferences.autoImportFileExcludePatterns": [
        "./ark/schema/api.ts"
    ]

However, if I do this, I no longer get autoimports from @arktype/schema in arktype as @arktype/schema, but rather from the nested file, even though it is not exported in @arktype/schema's package.json.

Maybe it is because I only have a single root-level tsconfig that is shared by both packages during dev?

EDIT: it seems like this happens even if I create a tsconfig.json in each individual package.

andrewbranch commented 2 months ago

If you rename api.ts to index.ts and delete the autoImportFileExcludePatterns you should be good to go šŸ˜¬

ssalbdivad commented 2 months ago

@andrewbranch Is index.ts hardcoded? šŸ˜…

I wanted to avoid it because of the association with directory imports but I would definitely switch back if it is the only way to get that behavior for now.

andrewbranch commented 2 months ago

https://github.com/microsoft/TypeScript/blob/4ada2706a76f4e22867c10830ac4add52af34e2c/src/services/codefixes/importFixes.ts#L1344-L1358

It was originally intended primarily to avoid module specifiers like "../.." because they look code smelly, but it was later expanded slightly to deal with circularities in more cases. It might make sense to remove the index filename part of this now.

ssalbdivad commented 2 months ago

@andrewbranch I think that works! šŸŽ‰ Thank you so much- that has been plaguing me since long before I commented on this a year ago šŸ˜…

Would definitely be interested if other file names are supported so I can switch back, but I care 95% less about that than being able to import across packages easily.

andrewbranch commented 2 months ago

It actually might be possible to do this cheaply without heuristics, as long as the file youā€™re currently editing exports something... the data structure central to auto imports can answer whether an arbitrary symbol exported from your current file can also be imported from the file weā€™re generating an import to, detecting a circularity. Weā€™re well set up to do that for individual exports, just not whole modules, so weā€™d do well for export * but could fail to detect cases like export { JustOneThing } from "./foo"...

ssalbdivad commented 2 months ago

I'm generally exporting a specific subset of symbols in a package entry point to make the API as explicit as possible.

The index.ts has such marginal downside I'm unlikely to think about this again unless I specifically read somewhere that other files can be handled the same way by autoimports.

I guess the biggest downside is discoverability as I never would have guessed changing the file name would fix this.