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.

Sign up for GitHub

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

Jump to bottom

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

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

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2ffcc2b
`extensions`: use the `extensionAlias` option
phryneas Jul 3, 2023
e619e59
Update src/rules/extensions.js
phryneas Jul 3, 2023
d8482f4
PR feedback
phryneas Jul 5, 2023
ccf5f3c
Merge remote-tracking branch 'upstream/main' into pr/typescript-alias…
phryneas Jul 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 38 additions & 1 deletion src/rules/extensions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,8 @@ module.exports = {
context,
) || isScoped(importPath);

if (!extension || !importPath.endsWith(`.${extension}`)) {
const validExtensions = getValidExtensionFor(context, importPath, extension);
if (!extension || !validExtensions.some((extension) => importPath.endsWith(`.${extension}`))) {
// ignore type-only imports and exports
if (node.importKind === 'type' || node.exportKind === 'type') { return; }
const extensionRequired = isUseOfExtensionRequired(extension, isPackage);
Expand All @@ -190,3 +191,39 @@ module.exports = {
return moduleVisitor(checkFileExtension, { commonjs: true });
},
};

/**
* 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)
*/
Comment on lines +196 to +204
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 •
edited
Loading

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.

const defaultExtensionAlias = {
'.js': [
'.ts',
// `.tsx` can also be compiled as `.js`
'.tsx',
'.d.ts',
'.js',
],
'.jsx': ['.tsx', '.d.ts', '.jsx'],
'.cjs': ['.cts', '.d.cts', '.cjs'],
'.mjs': ['.mts', '.d.mts', '.mjs'],
};

function getValidExtensionFor(context, importPath, resolvedExtension) {
let extensionAlias = {};
if (context.settings['import/resolver'] && context.settings['import/resolver'].typescript) {
extensionAlias = context.settings['import/resolver'].typescript.extensionAlias || defaultExtensionAlias;
}

const importedExtension = path.extname(importPath);
if (importedExtension in extensionAlias) {
phryneas marked this conversation as resolved.
Show resolved Hide resolved
return extensionAlias[importedExtension].map((ext) => ext.substring(1));
phryneas marked this conversation as resolved.
Show resolved Hide resolved
}
return [ resolvedExtension ];
phryneas marked this conversation as resolved.
Show resolved Hide resolved
}

1 change: 1 addition & 0 deletions tests/files/typescript-tsx.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const foo = <div>Foo</div>;
1 change: 1 addition & 0 deletions tests/files/typescript-with-index/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const foo = "foo";
68 changes: 68 additions & 0 deletions tests/src/rules/extensions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,15 @@ import rule from 'rules/extensions';
import { getTSParsers, test, testFilePath, parsers } from '../utils';

const ruleTester = new RuleTester();
const ruleTesterWithTypeScriptImports = new RuleTester({
settings: {
'import/resolver': {
typescript: {
alwaysTryTypes: true,
},
},
},
});

ruleTester.run('extensions', rule, {
valid: [
Expand Down Expand Up @@ -642,5 +651,64 @@ describe('TypeScript', () => {
}),
],
});

ruleTesterWithTypeScriptImports.run(`${parser}: allow importing JS extension when a TS file is resolved`, rule, {
valid: [
test({
code: 'import { foo } from "./typescript.js";',
options: [
'always',
],
}),
test({
code: 'import { foo } from "./typescript-tsx.jsx";',
options: [
'always',
],
}),
test({
code: 'import { foo } from "./typescript-tsx.js";',
options: [
'always',
],
}),
test({
code: 'import { foo } from "./typescript-with-index/index.js";',
options: [
'always',
],
}),
test({
code: 'import { foo } from "./typescript.js";',
options: [
'always',
{ ts: 'never', tsx: 'never', js: 'always', jsx: 'always' },
],
}),
],
invalid: [
test({
code: 'import { foo } from "./typescript";',
errors: ['Missing file extension "ts" for "./typescript"'],
options: [
'always',
],
}),
test({
code: 'import { foo } from "./typescript-tsx";',
errors: ['Missing file extension "tsx" for "./typescript-tsx"'],
options: [
'always',
],
}),
test({
code: 'import { foo } from "./typescript-with-index";',
errors: ['Missing file extension "ts" for "./typescript-with-index"'],
options: [
'always',
],
}),
],
});
});
});