un-ts / eslint-plugin-import-x

`eslint-plugin-import-x` is a fork of `eslint-plugin-import` that aims to provide a more performant and more lightweight version of the original plugin.
https://npm.im/eslint-plugin-import-x
MIT License
434 stars 19 forks source link

add `eslint-import-resolver-typescript` as a dependency #150

Open Zamiell opened 2 months ago

Zamiell commented 2 months ago

From @silverwind:

I think eslint-import-resolver-typescript is currently only mandatory for typescript, but I think we should make it a dependency and default to it for both JS and TS. The default JS resolver has too many issues like not supporting exports object format which are resolved in the typescript resolver.

(Originally in this pull request.)

Doing this would simplify the config for the end-user, which would be a big win!

cc @SukkaW @michaelfaith

SukkaW commented 2 months ago

There are still a few serious issues with eslint-import-resolver-typescript.

Also I am putting my bet on https://github.com/9romise/eslint-import-resolver-oxc as well.

michaelfaith commented 2 months ago

Thougts about absorbing typescript parser into the typescript flat config, too? That'd add an optional peer dependency on @typescript-eslint/parser, but remove the need for users to have to define a parser.

SukkaW commented 2 months ago

That'd add an optional peer dependency

I don't know how the package managers would handle this. It seems that yarn 1, yarn@berry, npm@5, npm@latest, pnpm@6, and pnpm@latest all handle optional peer dependencies differently.

Also, since most package managers don't hoist the transitive dependencies.

michaelfaith commented 2 months ago

That'd add an optional peer dependency

I don't know how the package managers would handle this. It seems that yarn 1, yarn@berry, npm@5, npm@latest, pnpm@6, and pnpm@latest all handle optional peer dependencies differently.

Also, since most package managers don't hoist the transitive dependencies.

Not sure I follow. All three package managers support peerDependenciesMeta.optional (yarn 1 & berry, npm since v7 / node 15, and pnpm). It wouldn't be a transitive dependency though. The consuming project would still need to have it installed as a first class dep, if they're using the typescript config. (optional peer dependency, not optional dependency)

SukkaW commented 2 months ago

It wouldn't be a transitive dependency though.

What happens if someone is publishing his eslint presets to npm (e.g. @antfu/eslint-config and eslint-config-sukka)? Then eslint-import-resolver-typescript becomes a transitive dependency of that preset package. Even if eslint-import-resolver-typescript is listed as a direct dependency of that preset package, it is still a transitive dependency (current project -> eslint-config-presets => eslint-import-resolver-typescript).

This used to cause me shit loads of issues when I am building eslint-config-sukka.

Zamiell commented 2 months ago

It wouldn't be a transitive dependency though.

What happens if someone is publishing his eslint presets to npm (e.g. @antfu/eslint-config and eslint-config-sukka)? Then eslint-import-resolver-typescript becomes a transitive dependency of that preset package. Even if eslint-import-resolver-typescript is listed as a direct dependency of that preset package, it is still a transitive dependency (current project -> eslint-config-presets => eslint-import-resolver-typescript).

This used to cause me shit loads of issues when I am building eslint-config-sukka.

I tried this over the weekend, and with the npm package manager specifically, it won't work properly if the dependency chain is like:

my-project --> my-linting-metapackage --> eslint-config-foo --> eslint-plugin-import-x & eslint-import-resolver-typescript

However, the dependency chain DOES work just fine if the dependency chain is like:

my-project --> my-linting-metapackage --> eslint-config-foo & eslint-import-resolver-typescript

I didn't test any other package managers than npm.

Thus, I think there is still value in putting eslint-import-resolver-typescript as a direct dependency of eslint-plugin-import-x, since some people (or maybe most people? unsure) will not consume eslint-import-plugin-x through a linting metapackage that nests the eslint-import-resolver-typescript dep more than two layers deep, if that makes sense. (And you could add this limitation to the readme.)

error-four-o-four commented 2 months ago

Hey there,

I was wondering if it would make sense to add support for (conditional) exports fields in package json files to the (default) node resolver instead of introducing another dependency?

imho the only purpose of eslint-import-resolver-typescript should be to resolve .ts, .d.ts files, aliased and project paths

silverwind commented 2 months ago

I was wondering if it would make sense to add support for (conditional) exports fields in package json files to the (default) node resolver instead of introducing another dependency?

It would, but the counter-argument is that maintaining two resolvers will not go well in the long run and it's better to focus efforts on making one resolver good for everything, even at the cost of additional dependencies, which are often not much of a cost to bear.

error-four-o-four commented 2 months ago

Please let me know if this is unrelated or not the right place (as there also is #40) or if I'm wrong. eslint-import-resolver-node (and others) rely on resolve whereas eslint-import-resolver-typescript introduces enhanced-resolve. In my case (and I don't know if it's an edge case) I'm pretty sure that I won't be using webpack in any of my projects in the near future. Therefore I'm not enthusiastic about this package, but that's just an opinion. I didn't take a closer look at these packages regarding performance or benchmark but I'm sure that their doing a great job. Anyway your point is absolutely reasonable. The way I see it is, that it would be great to have some kind of extendable resolver. Like a monorepo with a 'core' package. Other resolver packages/workspaces (like ts, webpack, vue, whatever) could extend this core package and wouldn't have to rewrite/build core functionality again.