Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork1.6k
extensions: use theextensionAlias option ofimport-resolver-typescript#2813
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
of `import-resolver-typescript`
| /** | ||
| * 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) | ||
| */ |
There was a problem hiding this comment.
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 oldermajor 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?
There was a problem hiding this comment.
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 anydependency oneslint-import-resolver-typescript yet, but adevDependency on1.0.0 || 1.1.0 - the current version is3.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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 anpeerDependency withpeerDependenciesMetaoptional: true.
If the package is there, use it, if it isn't we should avoid the relevant codeblock anyways.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 thesettings['import/resolver'].typescript setting?
If that's set, the import plugin will use the TS resolver, otherwise not.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 thenode16 target, for at least the last 5 releases of TypeScript, and it is not supported by theimports/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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Jordan Harband <ljharb@gmail.com>
Little heads up: It's already late here, and tomorrow is public holiday, so I'll probably add the requested changes on Wednesday. |
Changes are in from my side :) |
This comment was marked as off-topic.
This comment was marked as off-topic.
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) |
This comment was marked as spam.
This comment was marked as spam.
I'm fine to share a package but it seems the version strategy is very different between |
You wouldn’t have to, if the extracted package does, it’d work for both. |
niksy commentedMay 19, 2024
Anything we can do to get this merged? |
Still waiting to figure out if we can share a common package (matching this plugin's version support). |
niksy commentedMay 20, 2024
What do we expect for shared package to do? Export list of extension mappings? |
TypeScript with
moduleResolution: "node16"requires imports likeimport { 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/extensionsrule if theeslint-import-resolver-typescriptis set up - the resolver will return the original file name with the.tsextension, and the eslint rule will error since"js" !== "ts".This PR modifies the
import/extensionsrule to take theextensionAliassetting into account if thetypescriptresolver is configured.Related issues:#2729 and#2776