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
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 40 additions & 2 deletions src/rules/extensions.js
Expand Up @@ -4,6 +4,7 @@ import resolve from 'eslint-module-utils/resolve';
import { isBuiltIn, isExternalModule, isScoped } from '../core/importType';
import moduleVisitor from 'eslint-module-utils/moduleVisitor';
import docsUrl from '../docsUrl';
import has from 'has';

const enumValues = { enum: [ 'always', 'ignorePackages', 'never' ] };
const patternProperties = {
Expand Down Expand Up @@ -156,7 +157,7 @@ module.exports = {

// get extension from resolved path, if possible.
// for unresolved, use source value.
const extension = path.extname(resolvedPath || importPath).substring(1);
const extension = path.extname(resolvedPath || importPath).slice(1);

// determine if this is a module
const isPackage = isExternalModule(
Expand All @@ -165,7 +166,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 +192,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
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
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
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
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
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.

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 (has(extensionAlias, importedExtension)) {
return extensionAlias[importedExtension].map((ext) => ext.slice(1));
}
return [resolvedExtension];
}

1 change: 1 addition & 0 deletions tests/files/typescript-tsx.tsx
@@ -0,0 +1 @@
export const foo = <div>Foo</div>;
1 change: 1 addition & 0 deletions tests/files/typescript-with-index/index.ts
@@ -0,0 +1 @@
export const foo = "foo";
68 changes: 68 additions & 0 deletions tests/src/rules/extensions.js
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',
],
}),
],
});
});
});