Skip to content

Enhance import-style rule with auto-fix capability #2528

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

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
4a9bc16
Enhance `import-style` rule with auto-fix capability
CasperEngl Jan 17, 2025
98598d5
Refactor `import-style` rule: Remove `autoFix` option and update docu…
CasperEngl Jan 17, 2025
2fc70ec
Enhance `import-style` rule with improved namespace handling and test…
CasperEngl Jan 17, 2025
0abcc93
Fix references to the namespace imports
CasperEngl Jan 20, 2025
49b7cd7
Rename test/import-style.js to test/import-style.mjs and add special …
CasperEngl Jan 20, 2025
e4a832b
Refactor import-style.js
CasperEngl Jan 20, 2025
e74db1f
Refactor import styles configuration and handling
CasperEngl Jan 20, 2025
9536269
Refactor import and dynamic import rules
CasperEngl Jan 20, 2025
c632b34
Move create method back into root scope
CasperEngl Jan 22, 2025
53b9d13
Refactor specialCases object in import-style.js
CasperEngl Jan 22, 2025
9ef18bd
Fix node property keys
CasperEngl Jan 22, 2025
c9e24ee
Add tests for scoped packages, relative imports with hashes and exten…
CasperEngl Jan 22, 2025
8c5fdc5
improve namespace identifier generation for scoped packages
CasperEngl Jan 22, 2025
7588a95
move getNamespaceIdentifier function to module scope
CasperEngl Jan 22, 2025
36f1a03
rename variables to be more descriptive in import specifiers handling
CasperEngl Jan 22, 2025
6c152fa
fix code formatting and spacing in import-style.js
CasperEngl Jan 22, 2025
3374c91
optimize regex
CasperEngl Jan 22, 2025
db3699d
Refactor import-style.js and add test cases for numeric module names
CasperEngl Jan 23, 2025
a2239f3
Add createFix function to handle namespace imports in import-style.js
CasperEngl Jan 23, 2025
51c0ddf
Revert to use context.on handlers
CasperEngl Jan 23, 2025
82d41e3
Move schema to module scope
CasperEngl Jan 23, 2025
1268fd8
Use correct type
CasperEngl Jan 23, 2025
f2af0eb
Remove unnecessary imports for moment and date-fns libraries
CasperEngl Jan 23, 2025
87a1ad7
Trim trailing slashes in import paths
CasperEngl Jan 23, 2025
a8fd20e
Refactor getNamespaceIdentifier to use lastPart instead of packageName
CasperEngl Jan 23, 2025
c7b350c
Refactor createFix function to only create a fix if allowedImportStyl…
CasperEngl Jan 23, 2025
6b1e679
Create fix generator for converting imports to namespace imports
CasperEngl Jan 23, 2025
cfe9cc8
Refactor avoidCapture to use getAvailableVariableName
CasperEngl Jan 23, 2025
43ff202
Add blank line
CasperEngl Jan 23, 2025
3dba112
Refactor
fisker Jan 24, 2025
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
2 changes: 2 additions & 0 deletions docs/rules/import-style.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

💼 This rule is enabled in the ✅ `recommended` [config](https://github.com/sindresorhus/eslint-plugin-unicorn#preset-configs-eslintconfigjs).

🔧 This rule is automatically fixable by the [`--fix` CLI option](https://eslint.org/docs/latest/user-guide/command-line-interface#--fix).

<!-- end auto-generated rule header -->
<!-- Do not manually modify this header. Run: `npm run fix:eslint-docs` -->

Expand Down
2 changes: 1 addition & 1 deletion readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ export default [
| [expiring-todo-comments](docs/rules/expiring-todo-comments.md) | Add expiration conditions to TODO comments. | ✅ | | |
| [explicit-length-check](docs/rules/explicit-length-check.md) | Enforce explicitly comparing the `length` or `size` property of a value. | ✅ | 🔧 | 💡 |
| [filename-case](docs/rules/filename-case.md) | Enforce a case style for filenames. | ✅ | | |
| [import-style](docs/rules/import-style.md) | Enforce specific import styles per module. | ✅ | | |
| [import-style](docs/rules/import-style.md) | Enforce specific import styles per module. | ✅ | 🔧 | |
| [new-for-builtins](docs/rules/new-for-builtins.md) | Enforce the use of `new` for all builtins, except `String`, `Number`, `Boolean`, `Symbol` and `BigInt`. | ✅ | 🔧 | |
| [no-abusive-eslint-disable](docs/rules/no-abusive-eslint-disable.md) | Enforce specifying rules to disable in `eslint-disable` comments. | ✅ | | |
| [no-anonymous-default-export](docs/rules/no-anonymous-default-export.md) | Disallow anonymous functions and classes as the default export. | ✅ | | 💡 |
Expand Down
178 changes: 176 additions & 2 deletions rules/import-style.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {getStringIfConstant} from '@eslint-community/eslint-utils';
import {defaultsDeep} from './utils/lodash.js';
import {isCallExpression} from './ast/index.js';
import getAvailableVariableName from './utils/get-available-variable-name.js';
import {defaultsDeep} from './utils/lodash.js';

const MESSAGE_ID = 'importStyle';
const messages = {
Expand Down Expand Up @@ -127,6 +128,169 @@
},
};

const specialCases = {
react: 'React',
'react-dom': 'ReactDOM',
'react-router': 'ReactRouter',
'react-router-dom': 'ReactRouterDOM',
'prop-types': 'PropTypes',
lodash: '_',
'lodash-es': '_',
jquery: '$',
'styled-components': 'styled',
redux: 'Redux',
'react-redux': 'ReactRedux',
axios: 'Axios',
ramda: 'R',
rxjs: 'Rx',
vue: 'Vue',
angular: 'Angular',
};

const getNamespaceIdentifier = moduleName => {
// Handle special cases first
if (specialCases[moduleName]) {
return specialCases[moduleName];
}

// Trim trailing slashes and get the last part of the path and remove extension
const lastPart = moduleName.replace(/\/+$/, '').split('/').pop().split('.')[0];

// Replace invalid identifier characters and convert to camelCase
let identifier = lastPart
.replaceAll(/[^\dA-Za-z-]/g, '-') // Replace invalid chars with hyphen
.replaceAll(/-./g, x => x[1].toUpperCase()); // Convert to camelCase

// Prefix with underscore if starts with a number
if (/^\d/.test(identifier)) {
identifier = '_' + identifier;
}

return identifier;
};

/**
Creates a fix generator function to convert imports to namespace imports.
@param {Object} options - The options object.
@param {import('estree').Node} options.node - The AST node representing the import/require statement.
@param {import('eslint').SourceCode} options.sourceCode - The ESLint source code object.
@param {string} options.moduleName - The name of the module being imported.
@returns {(fixer: import('eslint').Rule.RuleFixer) => Generator<import('eslint').Rule.Fix, void, undefined> | undefined} A function that takes a fixer and returns a generator of fixes.
*/
const fix = ({node, sourceCode, moduleName, allowedImportStyles}) => {
// Currently we only fix named to namespace
if (!allowedImportStyles.has('namespace') || allowedImportStyles.has('named')) {
return;
}

const isImportDeclaration = node.type === 'ImportDeclaration';
const isVariableDeclarator = node.type === 'VariableDeclarator';
const isRequireCall = isCallExpression(node.init, {name: 'require'});
const isImportExpression = node.init?.type === 'ImportExpression';
const isVariableDeclaratorWithRequireCall = isVariableDeclarator && (isRequireCall || isImportExpression);
const isValidNode = isImportDeclaration || isVariableDeclaratorWithRequireCall;

if (!isValidNode) {
return;
}

let importedNames = [];
if (node.type === 'ImportDeclaration') {
importedNames = node.specifiers
.filter(specifier => specifier.type === 'ImportSpecifier' || specifier.type === 'ImportDefaultSpecifier')
.map(specifier => ({
localName: specifier.local.name,
importedName: specifier.type === 'ImportDefaultSpecifier' ? 'default' : specifier.imported.name,
}));
} else if (node.type === 'VariableDeclarator' && node.id.type === 'ObjectPattern') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can focus on imports, leave require() alone, ESM should be preferred.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, we no longer plan to handle anything CommonJS.

importedNames = node.id.properties
.map(property => {
if (property.type === 'RestElement') {
return {
localName: property.argument.name,
importedName: undefined,
};
}

return {
localName: property.value.name,
importedName: property.key.name,
};
});
}

if (importedNames.length === 0) {
return;
}

return function * (fixer) {
const scope = sourceCode.getScope(node);

const namespaceIdentifier = getNamespaceIdentifier(moduleName);

// Check if any of the named imports match our desired namespace identifier
const hasMatchingNamedImport = importedNames.some(
({localName}) => localName === namespaceIdentifier,
);

// Only avoid capture if there's no matching named import
const uniqueNamespaceIdentifier = hasMatchingNamedImport
? namespaceIdentifier
: getAvailableVariableName(namespaceIdentifier, [scope]);

// For VariableDeclarator, we need to handle the parent VariableDeclaration
const hasSemicolon = sourceCode.getText(
node.type === 'VariableDeclarator' ? node.parent : node,
).endsWith(';');

yield fixer.replaceTextRange(
node.type === 'VariableDeclarator'
? [node.parent.range[0], node.parent.range[1]]
: [node.range[0], node.range[1]],
node.type === 'ImportDeclaration'
? `import * as ${uniqueNamespaceIdentifier} from ${sourceCode.getText(node.source)}${hasSemicolon ? ';' : ''}`
: `const ${uniqueNamespaceIdentifier} = require(${sourceCode.getText(
node.type === 'VariableDeclarator' ? node.init.arguments[0] : node.arguments[0],
)})${hasSemicolon ? ';' : ''}`,
);

if (hasMatchingNamedImport) {
return;
}

for (const {localName, importedName} of importedNames) {
// Skip rest patterns since they should be kept as is
if (importedName === undefined) {
continue;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please help me understand this. Since you turn the VariableDeclarator into an identifier, so the RestElement just removed?


const programScope = sourceCode.getScope(sourceCode.ast);

const getAllReferences = scope => {
Copy link
Collaborator

@fisker fisker Jan 24, 2025

Choose a reason for hiding this comment

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

This part should use sourceCode.getDeclaredVariables() to get variable from the ImportDeclaration and then we have all the references. Including child scopes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

consistent-assert has similar logic, you can take a look. https://github.com/sindresorhus/eslint-plugin-unicorn/pull/2535/files

let references = scope.references.filter(
reference => reference.identifier.name === localName
// Skip the original declaration
&& !(node.type === 'VariableDeclarator' && reference.identifier === node.id)
&& !(node.type === 'VariableDeclarator' && node.id.type === 'ObjectPattern'
&& node.id.properties.some(p => p.value === reference.identifier)),
);

for (const childScope of scope.childScopes) {
references = [...references, ...getAllReferences(childScope)];
}

return references;
};

const references = getAllReferences(programScope);

for (const reference of references) {
yield fixer.replaceText(reference.identifier, `${uniqueNamespaceIdentifier}.${importedName}`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not the current fix.

import {bar} from 'foo'

a = {bar}

will fix to

import foo from 'foo'

a = {foo.bar} // Syntax error

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use the existing renameVariable utility.

}
}
};
};

/** @param {import('eslint').Rule.RuleContext} context */
const create = context => {
let [
Expand All @@ -153,7 +317,13 @@

const {sourceCode} = context;

const report = (node, moduleName, actualImportStyles, allowedImportStyles, isRequire = false) => {
const report = (
node,
moduleName,
actualImportStyles,
allowedImportStyles,
isRequire = false,
) => {

Check warning on line 326 in rules/import-style.js

View workflow job for this annotation

GitHub Actions / lint-test (ubuntu-latest)

Arrow function has too many parameters (5). Maximum allowed is 4

Check warning on line 326 in rules/import-style.js

View workflow job for this annotation

GitHub Actions / lint-test (windows-latest)

Arrow function has too many parameters (5). Maximum allowed is 4
if (!allowedImportStyles || allowedImportStyles.size === 0) {
return;
}
Expand Down Expand Up @@ -182,6 +352,9 @@
node,
messageId: MESSAGE_ID,
data,
fix: fix({
node, sourceCode, moduleName, allowedImportStyles,
}),
});
};

Expand Down Expand Up @@ -364,6 +537,7 @@
description: 'Enforce specific import styles per module.',
recommended: true,
},
fixable: 'code',
schema,
defaultOptions: [{}],
messages,
Expand Down
Loading
Loading