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

extensions: use the extensionAlias option of import-resolver-typescript #2813

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

phryneas
Copy link
Contributor

@phryneas phryneas commented Jul 3, 2023

TypeScript with moduleResolution: "node16" requires imports like import { foo } from './tsFile.js' while on the disk, there is actually a ./tsFile.ts.
The idea here is that a module import needs a file extension and that TypeScript will not change the import statement during transpilation.

Currently, this does not work with the import/extensions rule if the eslint-import-resolver-typescript is set up - the resolver will return the original file name with the .ts extension, and the eslint rule will error since "js" !== "ts".

This PR modifies the import/extensions rule to take the extensionAlias setting into account if the typescript resolver is configured.

Related issues: #2729 and #2776

of `import-resolver-typescript`
Comment on lines +195 to +203
/**
* Taken from `eslint-import-resolver-typescript`.
* This could be imported from current versions of that plugin,
* but this project still depends on an older version.
* Also, importing it would add a dependency, or at least an
* optional peer dependency - copying the code seems like the
* more sane option.
* [LICENSE](https://github.com/import-js/eslint-import-resolver-typescript/blob/71b23a206514842fef70a99220e5ffb1d6da2a0e/LICENSE)
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it that we depend on an older major of that plugin? or that we depend on an older minor or patch?

Probably a better option would be extracting the code into its own package, that both we and eslint-import-resolver-typescript use. @JounQin, thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eslint-plugin-import doesn't have any dependency on eslint-import-resolver-typescript yet, but a devDependency on 1.0.0 || 1.1.0 - the current version is 3.5.5.
I didn't want to add a new dependency here just for that one config object, but I'm happy with whatever solution we end up with here :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, true enough. adding a dependency is fine, but perhaps the resolver's not ideal to add. hopefully an extracted package is viable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that doesn't work, we could go for an peerDependency with peerDependenciesMeta optional: true.
If the package is there, use it, if it isn't we should avoid the relevant codeblock anyways.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to rely on optional peers; any npm client older than that feature would treat it as required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Then let's see what @JounQin has to say :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be a setting of import plugin to for using without TypeScript resolver? For example, could webpack resolver resolves same path?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's already the settings['import/resolver'].typescript setting?
If that's set, the import plugin will use the TS resolver, otherwise not.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://webpack.js.org/configuration/resolve/#resolveextensionalias

I mean the webpack resolver could also resolve different extensions if it's working correctly? Then only relying on the typescript resolver's setting seems incorrect to me.

Copy link
Contributor Author

@phryneas phryneas Jul 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe? I don't know what that one does, or if anyone ever had this problem with it.
The problem at hand is that this is something TypeScript expects with the node16 target, for at least the last 5 releases of TypeScript, and it is not supported by the imports/extensions rule. There doesn't seem any way of asking the resolver "is this a valid replacement file extension for this other extension", since the resolver is pretty much hidden from the rule, so I don't know a way of implementing this better.

src/rules/extensions.js Outdated Show resolved Hide resolved
src/rules/extensions.js Outdated Show resolved Hide resolved
src/rules/extensions.js Outdated Show resolved Hide resolved
Co-authored-by: Jordan Harband <ljharb@gmail.com>
@phryneas
Copy link
Contributor Author

phryneas commented Jul 3, 2023

Little heads up: It's already late here, and tomorrow is public holiday, so I'll probably add the requested changes on Wednesday.

@phryneas
Copy link
Contributor Author

phryneas commented Jul 5, 2023

Changes are in from my side :)

@Patryk-B

This comment was marked as off-topic.

@ljharb ljharb marked this pull request as draft July 25, 2023 22:56
@ljharb
Copy link
Member

ljharb commented Jul 25, 2023

I've rebased this; if all tests pass, then the only open question seems to be whether we can extract this logic into a package that is shared by both this plugin and the typescript resolver (cc @JounQin)

@beamery-tomht

This comment was marked as spam.

@JounQin
Copy link
Collaborator

JounQin commented Dec 6, 2023

I've rebased this; if all tests pass, then the only open question seems to be whether we can extract this logic into a package that is shared by both this plugin and the typescript resolver (cc @JounQin)

I'm fine to share a package but it seems the version strategy is very different between eslint-plugin-import vs eslint-import-resolver-typescript. (I don't want to support old Node versions.)

@ljharb
Copy link
Member

ljharb commented Dec 6, 2023

You wouldn’t have to, if the extracted package does, it’d work for both.

@niksy
Copy link

niksy commented May 19, 2024

Anything we can do to get this merged?

@ljharb
Copy link
Member

ljharb commented May 19, 2024

Still waiting to figure out if we can share a common package (matching this plugin's version support).

@niksy
Copy link

niksy commented May 20, 2024

What do we expect for shared package to do?

Export list of extension mappings?
Resolve to mapped extension if current context has Typescript configured for import/resolver?
What should it be named?

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

Successfully merging this pull request may close these issues.

6 participants