Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Zamiell opened this issue Sep 7, 2024 · 9 comments
Open

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

Zamiell opened this issue Sep 7, 2024 · 9 comments

Comments

@Zamiell
Copy link

Zamiell commented Sep 7, 2024

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
Copy link
Collaborator

SukkaW commented Sep 7, 2024

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.

@Zamiell Zamiell changed the title feat: add eslint-import-resolver-typescript as a dependency add eslint-import-resolver-typescript as a dependency Sep 7, 2024
@michaelfaith
Copy link

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
Copy link
Collaborator

SukkaW commented Sep 8, 2024

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
Copy link

michaelfaith commented Sep 9, 2024

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
Copy link
Collaborator

SukkaW commented Sep 10, 2024

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
Copy link
Author

Zamiell commented Sep 10, 2024

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
Copy link

error-four-o-four commented Sep 10, 2024

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
Copy link

silverwind commented Sep 10, 2024

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
Copy link

error-four-o-four commented Sep 10, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants