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

feat: add enforceEsmExtensions option to import/extensions rule #2701

Closed
wants to merge 12 commits into from
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ This plugin intends to support linting of ES2015+ (ES6+) import/export syntax, a
| [consistent-type-specifier-style](docs/rules/consistent-type-specifier-style.md) | Enforce or ban the use of inline type-only markers for named imports. | | | | 🔧 | | |
| [dynamic-import-chunkname](docs/rules/dynamic-import-chunkname.md) | Enforce a leading comment with the webpackChunkName for dynamic imports. | | | | | | |
| [exports-last](docs/rules/exports-last.md) | Ensure all exports appear after other statements. | | | | | | |
| [extensions](docs/rules/extensions.md) | Ensure consistent use of file extension within the import path. | | | | | | |
| [extensions](docs/rules/extensions.md) | Ensure consistent use of file extension within the import path. | | | | 🔧 | 💡 | |
Copy link
Member

Choose a reason for hiding this comment

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

if we're going to make it autofixable, let's make sure it can autofix everything, not just this new option?

it might be nice to have a separate PR that makes the rule autofixable?

Copy link
Author

Choose a reason for hiding this comment

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

What I saw from adding the tests for this option is that all the "valid" usecases where imports are fixed will need updating, so the PR could quickly become very large.

I would much rather open a separate PR for autofixing everything.

Copy link
Author

Choose a reason for hiding this comment

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

to be fair though, many of the failures don't have valid fixes. i.e. import not found... how do we want to handle those? remove the import?

| [first](docs/rules/first.md) | Ensure all imports appear before other statements. | | | | 🔧 | | |
| [group-exports](docs/rules/group-exports.md) | Prefer named exports to be grouped together in a single export declaration | | | | | | |
| [imports-first](docs/rules/imports-first.md) | Replaced by `import/first`. | | | | 🔧 | | ❌ |
Expand Down
30 changes: 29 additions & 1 deletion docs/rules/extensions.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# import/extensions

🔧💡 This rule is automatically fixable by the [`--fix` CLI option](https://eslint.org/docs/latest/user-guide/command-line-interface#--fix) and manually fixable by [editor suggestions](https://eslint.org/docs/developer-guide/working-with-rules#providing-suggestions).

<!-- end auto-generated rule header -->

Some file resolve algorithms allow you to omit the file extension within the import source path. For example the `node` resolver can resolve `./foo/bar` to the absolute path `/User/someone/foo/bar.js` because the `.js` extension is resolved automatically by default. Depending on the resolver you can configure more extensions to get resolved automatically.
Expand Down Expand Up @@ -38,14 +40,22 @@ By providing both a string and an object, the string will set the default settin

For example, `["error", "never", { "svg": "always" }]` would require that all extensions are omitted, except for "svg".

`ignorePackages` can be set as a separate boolean option like this:
## Additional Options

This rule provides additional options for configuration:

- `ignorePackages` follows the same logic as setting the string value of the rule config to `ignorePackages`. Useful when using `always` as the string config.
- `enforceEsmExtensions` will flag any relative import paths that do not resolve to a file. (supports --fix)
Copy link
Member

Choose a reason for hiding this comment

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

maybe instead of hardcoding ESM here (or java-ly capitalizing it "Esm"), this could be an option that takes always or never, and solely applies to relative paths?

Meaning, relativePaths: "always" would be the node native ESM approach (since that and Deno are the only environments where extensions are required for native ESM - browsers don't, and any transpiled ESM shouldn't).

Copy link
Author

@SgtPooki SgtPooki Feb 8, 2023

Choose a reason for hiding this comment

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

java-ly..

hah.. "ESME xtensions" isn't any better ;)

yea it should only apply to relative paths. I am fairly certain that I plugged it into the existing rule where that is true but let me know if I missed something.

Since we're already talking about extensions an option with extensions in the name is superfluous. I'm fine with 'relativePaths', but also thought about using 'fullySpecified' similar to webpack's resolver naming: https://webpack.js.org/configuration/module/#resolvefullyspecified


`ignorePackages` and `enforceEsmExtensions` can be set as a separate boolean option like this:

```
"import/extensions": [
<severity>,
"never" | "always" | "ignorePackages",
{
ignorePackages: true | false,
enforceEsmExtensions: true | false,
pattern: {
<extension>: "never" | "always" | "ignorePackages"
}
Expand Down Expand Up @@ -167,6 +177,24 @@ import express from 'express';
import foo from '@/foo';
```

The following patterns are considered problems when configuration set to `['error', 'always', {ignorePackages: true, enforceEsmExtensions: true} ]`:

```js
import foo from './foo'
import bar from './bar'
import Component from './Component'
import express from 'express'
```

The following patterns are not considered problems when configuration set to `['error', 'always', {ignorePackages: true, enforceEsmExtensions: true} ]`:

```js
import foo from './foo.js'
import bar from './bar/index.js'
import Component from './Component.js'
import express from 'express'
```

## When Not To Use It

If you are not concerned about a consistent usage of file extension.
124 changes: 116 additions & 8 deletions src/rules/extensions.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import path from 'path';
import fs from 'fs';

import resolve from 'eslint-module-utils/resolve';
import { isBuiltIn, isExternalModule, isScoped } from '../core/importType';
Expand All @@ -15,6 +16,7 @@ const properties = {
properties: {
'pattern': patternProperties,
'ignorePackages': { type: 'boolean' },
'enforceEsmExtensions': { type: 'boolean' },
},
};

Expand All @@ -24,6 +26,7 @@ function buildProperties(context) {
defaultConfig: 'never',
pattern: {},
ignorePackages: false,
enforceEsmExtensions: false,
};

context.options.forEach(obj => {
Expand All @@ -35,7 +38,7 @@ function buildProperties(context) {
}

// If this is not the new structure, transfer all props to result.pattern
if (obj.pattern === undefined && obj.ignorePackages === undefined) {
if (obj.pattern === undefined && obj.ignorePackages === undefined && obj.enforceEsmExtensions === undefined) {
Object.assign(result.pattern, obj);
return;
}
Expand All @@ -49,6 +52,10 @@ function buildProperties(context) {
if (obj.ignorePackages !== undefined) {
result.ignorePackages = obj.ignorePackages;
}
// If enforceEsmExtensions is provided, transfer it to result
if (obj.enforceEsmExtensions !== undefined) {
result.enforceEsmExtensions = obj.enforceEsmExtensions;
}
});

if (result.defaultConfig === 'ignorePackages') {
Expand All @@ -59,6 +66,46 @@ function buildProperties(context) {
return result;
}

// util functions
const fileExists = function (filePath) {
try {
fs.accessSync(filePath, fs.constants.F_OK);
return true;
} catch (err) {
if (err && err.code) === 'ENOENT') {
// known and somewhat expected failure case.
return false;
}
return false;
}
};

const excludeParenthesisFromTokenLocation = function (token) {
if (token.range == null || token.loc == null) {
return token;
}
const rangeStart = token.range[0] + 1;
const rangeEnd = token.range[1] - 1;
const locColStart = token.loc.start.column + 1;
const locColEnd = token.loc.end.column - 1;
const newToken = {
...token,
range: [rangeStart, rangeEnd],
loc: {
start: { ...token.loc.start, column: locColStart },
end: { ...token.loc.end, column: locColEnd },
},
};

return newToken;
};

const getEsmImportFixer = function (tokenLiteral, updated) {
return function (fixer) {
return fixer.replaceText(excludeParenthesisFromTokenLocation(tokenLiteral), updated);
};
};

module.exports = {
meta: {
type: 'suggestion',
Expand All @@ -67,7 +114,7 @@ module.exports = {
description: 'Ensure consistent use of file extension within the import path.',
url: docsUrl('extensions'),
},

fixable: 'code',
schema: {
anyOf: [
{
Expand Down Expand Up @@ -103,6 +150,8 @@ module.exports = {
},
],
},

hasSuggestions: true,
},

create(context) {
Expand All @@ -114,13 +163,17 @@ module.exports = {
}

function isUseOfExtensionRequired(extension, isPackage) {
return getModifier(extension) === 'always' && (!props.ignorePackages || !isPackage);
return getModifier(extension) === 'always' && ((!props.ignorePackages || !isPackage) || props.enforceEsmExtensions) ;
}

function isUseOfExtensionForbidden(extension) {
return getModifier(extension) === 'never';
}

function isUseOfEsmImportsEnforced() {
return props.enforceEsmExtensions === true;
}

function isResolvableWithoutExtension(file) {
const extension = path.extname(file);
const fileWithoutExtension = file.slice(0, -extension.length);
Expand All @@ -137,6 +190,56 @@ module.exports = {
return false;
}

function getEsmExtensionReport(node) {

const esmExtensions = ['.js', '.ts', '.mjs', '.cjs'];
esmExtensions.push(...esmExtensions.map((ext) => `/index${ext}`));
Comment on lines +194 to +196
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const esmExtensions = ['.js', '.ts', '.mjs', '.cjs'];
esmExtensions.push(...esmExtensions.map((ext) => `/index${ext}`));
const esmExtensions = [].concat(
'.js',
'.mjs',
'.cjs',
esmExtensions.map((ext) => `/index${ext}`)
);

the .ts extension should definitely not be here since it's not ESM.

also, the .js extension isn't always ESM, only when type: module is present, so this might need to be more complex.

Copy link
Author

Choose a reason for hiding this comment

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

Your code isn't valid, but I can remove the 'ts' extension

w.r.t typescript extensions, here's the gotcha: in Typescript packages, you have to import foo from './foo.js' instead of import foo from './foo.ts'. These extensions are the extensions to check to validate that the path is real, but it should still be replacing a valid .ts with .js. Currently, the rule will actually set the import to '.ts' instead of '.js'. Which needs fixed...

For this option to be robust in the current landscape, we should really accept a mapping of expected fileExtensions, and the extension to use in the source. something like:

{
"import/extensions": ["error", "always", { 
    "esm": {
        "extensionOverride": { 
            "ts": "js", 
            "tsx": "jsx"
        }, 
        "extensions": ["js", "ts", "cjs", "mjs"]
    }}]
}

where extensions without overrides are replaced with themselves. What do you think about this.

Copy link
Author

Choose a reason for hiding this comment

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

also, the .js extension isn't always ESM, only when type: module is present, so this might need to be more complex.

I think for this rule, we can assume when someone enables it that they are using ESM for their non cjs files. Would a comment in the doc cover this usecase?

maybe changing the name to 'fullyResolved' would help communicate more accurately what it's fixing

Copy link
Author

Choose a reason for hiding this comment

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

relevant issue: microsoft/TypeScript#49083 (comment)

allowing overrides would let the consumers leave 'ts' replacements as 'ts' (deno migrations) or swapping ts for js (esm migration in typescript project)

Copy link
Member

Choose a reason for hiding this comment

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

what is invalid about that code? it works perfectly fine.

I understand about the TS-specific quirks but that should come from the TS resolver and not be hardcoded into a rule that might not be running on TS at all.

I do not think we should ever assume that someone is using type:module, which is harmful and should be avoided anyways (altho i believe TS's implementation requires it, not everyone uses TS).

Copy link
Member

Choose a reason for hiding this comment

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

rofl ok fair enough, good catch. in that case, let's keep it as is, but still without TS.

Suggested change
const esmExtensions = ['.js', '.ts', '.mjs', '.cjs'];
esmExtensions.push(...esmExtensions.map((ext) => `/index${ext}`));
const esmExtensions = ['.js', '.mjs', '.cjs'];
esmExtensions.push(...esmExtensions.map((ext) => `/index${ext}`));

however we'll still need to handle that .cjs isn't ESM, and .js is only ESM in type:module.

Unless this is "extensions that might exist in ESM files", and then i'm not sure why it wouldn't include wasm and json?

Copy link
Author

@SgtPooki SgtPooki Feb 16, 2023

Choose a reason for hiding this comment

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

Right, thats why im thinking we should change the rule to "fullyResolved" and then allow users to indicate the extensions they care about, as well as whatever mapping they need.

For my usecase (typescript) i have to check for the existence of both .js and .ts, and replace both with .js

What do you think of that?

Copy link
Member

Choose a reason for hiding this comment

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

tbh this really seems like a uniquely typescript use case, and it probably should live in the TS eslint plugin (cc @bradzacher).

Copy link
Contributor

@bradzacher bradzacher Feb 17, 2023

Choose a reason for hiding this comment

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

The philosophy of TS is and always has been that you write the import paths that you want at runtime. TS purposely does not transform import paths at all - it will always emit the exact same string after transpilation.

In fact currently it is an error to attempt to import a file using .ts because as far as TS was concerned - there's no "TS runtime", so importing a .ts file makes no sense! EG nodejs can't parse or understand TS, so importing a .ts file will crash things.

Because of this, when they added ESM support they added module resolution logic in the typechecker so that when TS sees ./foo.[cm]?jsx?, it will look for the file ./foo.[cm]?tsx? and will use that.

TS 5.0 includes a flag which makes it so that import './foo.ts' will no longer error for environments where they do understand TS syntax (eg deno or some bundlers). In this instance it will mean that TS will look for that exact file+extension.

To summarise:

  • ./foo will cause TS to do the standard node resolution logic, preferring ts extensions
    • eg it'll look for ./foo.ts, then ./foo.tsx, then ./foo.js, then ./foo/index.ts, etc
  • ./foo.[cm]?jsx? will cause TS to first look for the corresponding ./foo.[cm]?tsx? file first, then look for the originally specified file.
  • ./foo.[cm]?tsx? will cause TS to look for that exact file (with the correct config)

I think this is the priority order of how TS looks up extensions in the ambiguous cases:
https://github.com/microsoft/TypeScript/blob/ddfec78f55bfedd690708c429f803346555e31c4/src/compiler/utilities.ts#L9233-L9234

.d.ts, .d.mts, .d.cts, .mjs, .mts, .cjs, .cts, .ts, .js, .tsx, .jsx, .json

Copy link
Member

Choose a reason for hiding this comment

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

@bradzacher ok, so i'm not really sure what that means here.

It really seems like it should be an option to the TS resolver, not to any particular rule, but it's pretty complex :-)


const importSource = node.source;
const importedPath = importSource.value;
const cwd = context.getCwd();
const filename = context.getFilename();
const relativeFilePath = path.relative(cwd, filename);
const relativeSourceFileDir = path.dirname(relativeFilePath);
const absoluteSourceFileDir = path.resolve(cwd, relativeSourceFileDir);
const importedFileAbsolutePath = path.resolve(absoluteSourceFileDir, importedPath);
const importOrExportLabel = node.type.match(/import/i) != null ? 'import of' : 'export from';
let correctImportPath = null;
try {
for (let i = 0; i < esmExtensions.length; i++) {
const ext = esmExtensions[i];
const potentialImportPath = `${importedFileAbsolutePath}${ext}`;
if (fileExists(potentialImportPath, context)) {
correctImportPath = importedPath + ext;
break;
}
}
} catch (err) {
return null;
}
if (correctImportPath == null) {
return null;
}

if (correctImportPath.match(/^\./) == null) {
correctImportPath = `./${correctImportPath}`;
}
const suggestionDesc = `Use "${correctImportPath}" instead.`;
const fix = getEsmImportFixer(node.source, correctImportPath);

return {
message: `Invalid ESM ${importOrExportLabel} "${importedPath}". ${suggestionDesc}`,
node,
suggest: [
{
desc: suggestionDesc,
fix,
},
],
fix,
};
}

function checkFileExtension(source, node) {
// bail if the declaration doesn't have a source, e.g. "export { foo };", or if it's only partially typed like in an editor
if (!source || !source.value) return;
Expand Down Expand Up @@ -171,11 +274,16 @@ module.exports = {
const extensionRequired = isUseOfExtensionRequired(extension, isPackage);
const extensionForbidden = isUseOfExtensionForbidden(extension);
if (extensionRequired && !extensionForbidden) {
context.report({
node: source,
message:
`Missing file extension ${extension ? `"${extension}" ` : ''}for "${importPathWithQueryString}"`,
});
const esmExtensionsReport = isUseOfEsmImportsEnforced() ? getEsmExtensionReport(node) : null;
if (esmExtensionsReport != null) {
context.report(esmExtensionsReport);
} else {
context.report({
node: source,
message:
`Missing file extension ${extension ? `"${extension}" ` : ''}for "${importPathWithQueryString}"`,
});
}
}
} else if (extension) {
if (isUseOfExtensionForbidden(extension) && isResolvableWithoutExtension(importPath)) {
Expand Down
43 changes: 43 additions & 0 deletions tests/src/rules/extensions.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,16 @@ ruleTester.run('extensions', rule, {
options: [ 'never', { ignorePackages: true } ],
}),

test({
code: `
import foo from './foo.js'
import bar from './bar/index.js'
import Component from './Component.js'
import express from 'express'
`,
options: [ 'always', { enforceEsmExtensions: true, ignorePackages: true } ],
}),

test({
code: 'import exceljs from "exceljs"',
options: [ 'always', { js: 'never', jsx: 'never' } ],
Expand Down Expand Up @@ -357,6 +367,39 @@ ruleTester.run('extensions', rule, {
],
}),

test({
code: `
import testModule from './test-module'
import bar from './bar'
import Component from './Component'
import express from 'express'
`,
options: [ 'always', { enforceEsmExtensions: true, ignorePackages: true } ],
errors: [
{
message: 'Invalid ESM import of "./test-module". Use "./test-module/index.js" instead.',
line: 2,
column: 9,
},
{
message: 'Invalid ESM import of "./bar". Use "./bar.js" instead.',
line: 3,
column: 9,
},
{
message: 'Missing file extension for "./Component"',
line: 4,
column: 31,
},
],
output: `
import testModule from './test-module/index.js'
import bar from './bar.js'
import Component from './Component'
import express from 'express'
`,
}),

test({
code: `
import foo from './foo.js'
Expand Down