-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
base: main
Are you sure you want to change the base?
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 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?
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 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 :)
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 an peerDependency
with peerDependenciesMeta
optional: 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 the settings['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 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.
Co-authored-by: Jordan Harband <[email protected]>
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. |
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). |
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/extensions
rule if theeslint-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 theextensionAlias
setting into account if thetypescript
resolver is configured.Related issues: #2729 and #2776