From 911c25de59cefe6f77823258438123edb7f9299a Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sun, 5 Mar 2023 20:17:14 +0100 Subject: [PATCH] eslint-plugin: add auto fix for missing imports Signed-off-by: Patrik Oldsberg --- .changeset/twelve-parrots-camp.md | 5 + packages/eslint-plugin/lib/getPackages.js | 4 + packages/eslint-plugin/lib/visitImports.js | 30 ++- .../rules/no-undeclared-imports.js | 188 +++++++++++++----- .../src/no-undeclared-imports.test.ts | 64 +++++- 5 files changed, 232 insertions(+), 59 deletions(-) create mode 100644 .changeset/twelve-parrots-camp.md diff --git a/.changeset/twelve-parrots-camp.md b/.changeset/twelve-parrots-camp.md new file mode 100644 index 0000000000..ea7e254f29 --- /dev/null +++ b/.changeset/twelve-parrots-camp.md @@ -0,0 +1,5 @@ +--- +'@backstage/eslint-plugin': patch +--- + +Add support for auto-fixing missing imports detected by the `no-undeclared-imports` rule. diff --git a/packages/eslint-plugin/lib/getPackages.js b/packages/eslint-plugin/lib/getPackages.js index f5f2c63cc1..8f4ccfacf3 100644 --- a/packages/eslint-plugin/lib/getPackages.js +++ b/packages/eslint-plugin/lib/getPackages.js @@ -31,6 +31,7 @@ const manypkg = require('@manypkg/get-packages'); * @property {ExtendedPackage} root * @property {ExtendedPackage[]} list * @property {Map} map + * @property {() => void} clearCache * @property {(path: string) => ExtendedPackage | undefined} byPath */ @@ -64,6 +65,9 @@ module.exports = (function () { pkg => !path.relative(pkg.dir, filePath).startsWith('..'), ); }, + clearCache() { + result = undefined; + }, }; lastLoadAt = Date.now(); return result; diff --git a/packages/eslint-plugin/lib/visitImports.js b/packages/eslint-plugin/lib/visitImports.js index 1a344c1101..376c37575f 100644 --- a/packages/eslint-plugin/lib/visitImports.js +++ b/packages/eslint-plugin/lib/visitImports.js @@ -24,6 +24,16 @@ const getPackages = require('./getPackages'); * @type {object} * @property {'local'} type * @property {'value' | 'type'} kind + * @property {import('estree').Node} node + * @property {string} path + */ + +/** + * @typedef ImportDirective + * @type {object} + * @property {'directive'} type + * @property {'value' | 'type'} kind + * @property {import('estree').Node} node * @property {string} path */ @@ -32,6 +42,7 @@ const getPackages = require('./getPackages'); * @type {object} * @property {'internal'} type * @property {'value' | 'type'} kind + * @property {import('estree').Node} node * @property {string} path * @property {import('./getPackages').ExtendedPackage} package * @property {string} packageName @@ -42,6 +53,7 @@ const getPackages = require('./getPackages'); * @type {object} * @property {'external'} type * @property {'value' | 'type'} kind + * @property {import('estree').Node} node * @property {string} path * @property {string} packageName */ @@ -51,6 +63,7 @@ const getPackages = require('./getPackages'); * @type {object} * @property {'builtin'} type * @property {'value' | 'type'} kind + * @property {import('estree').Literal} node * @property {string} path * @property {string} packageName */ @@ -58,7 +71,7 @@ const getPackages = require('./getPackages'); /** * @callback ImportVisitor * @param {ConsideredNode} node - * @param {LocalImport | InternalImport | ExternalImport | BuiltinImport} import + * @param {ImportDirective | LocalImport | InternalImport | ExternalImport | BuiltinImport} import */ /** @@ -68,7 +81,7 @@ const getPackages = require('./getPackages'); /** * @param {ConsideredNode} node - * @returns {undefined | {path: string, kind: 'type' | 'value'}} + * @returns {undefined | {path: string, node: import('estree').Literal, kind: 'type' | 'value'}} */ function getImportInfo(node) { /** @type {import('estree').Expression | import('estree').SpreadElement | undefined | null} */ @@ -95,7 +108,11 @@ function getImportInfo(node) { /** @type {any} */ const anyNode = node; - return { path: pathNode.value, kind: anyNode.importKind ?? 'value' }; + return { + path: pathNode.value, + node: pathNode, + kind: anyNode.importKind ?? 'value', + }; } /** @@ -122,6 +139,10 @@ module.exports = function visitImports(context, visitor) { return visitor(node, { type: 'local', ...info }); } + if (info.path.startsWith('directive:')) { + return visitor(node, { type: 'directive', ...info }); + } + const pathParts = info.path.split('/'); // Check for match with plain name, then namespaced name @@ -143,6 +164,7 @@ module.exports = function visitImports(context, visitor) { return visitor(node, { type: 'builtin', kind: info.kind, + node: info.node, path: subPath, packageName, }); @@ -151,6 +173,7 @@ module.exports = function visitImports(context, visitor) { return visitor(node, { type: 'external', kind: info.kind, + node: info.node, path: subPath, packageName, }); @@ -159,6 +182,7 @@ module.exports = function visitImports(context, visitor) { return visitor(node, { type: 'internal', kind: info.kind, + node: info.node, path: subPath, package: pkg, packageName, diff --git a/packages/eslint-plugin/rules/no-undeclared-imports.js b/packages/eslint-plugin/rules/no-undeclared-imports.js index b1e11d1b97..06f0afed46 100644 --- a/packages/eslint-plugin/rules/no-undeclared-imports.js +++ b/packages/eslint-plugin/rules/no-undeclared-imports.js @@ -20,6 +20,7 @@ const path = require('path'); const getPackageMap = require('../lib/getPackages'); const visitImports = require('../lib/visitImports'); const minimatch = require('minimatch'); +const { execFileSync } = require('child_process'); const depFields = { dep: 'dependencies', @@ -124,11 +125,13 @@ function getAddFlagForDepsField(depsField) { module.exports = { meta: { type: 'problem', + fixable: 'code', messages: { undeclared: "{{ packageName }} must be declared in {{ depsField }} of {{ packageJsonPath }}, run 'yarn --cwd {{ packagePath }} add{{ addFlag }} {{ packageName }}' from the project root.", switch: '{{ packageName }} is declared in {{ oldDepsField }}, but should be moved to {{ depsField }} in {{ packageJsonPath }}.', + switchBack: 'Switch back to import declaration', }, docs: { description: @@ -150,63 +153,150 @@ module.exports = { return {}; } - return visitImports(context, (node, imp) => { - // We leave checking of type imports to the repo-tools check, - // and we skip builtins and local imports - if ( - imp.kind === 'type' || - imp.type === 'builtin' || - imp.type === 'local' - ) { - return; - } + /** @type Array<{name: string, flag: string, node: import('estree').Node}> */ + const importsToAdd = []; - // We skip imports for the package itself - if (imp.packageName === localPkg.packageJson.name) { - return; - } + return { + // All missing imports that we detect are collected as we traverse, and then we use + // the program exit to execute all install directives that have been found. + ['Program:exit']() { + /** @type Record> */ + const byFlag = {}; - const modulePath = path.relative(localPkg.dir, filePath); - const expectedType = getExpectedDepType( - localPkg.packageJson, - imp.packageName, - modulePath, - ); + for (const { name, flag } of importsToAdd) { + byFlag[flag] = byFlag[flag] ?? new Set(); + byFlag[flag].add(name); + } - const conflict = findConflict( - localPkg.packageJson, - imp.packageName, - expectedType, - ); + for (const name of byFlag[''] ?? []) { + byFlag['--dev']?.delete(name); + } + for (const name of byFlag['--peer'] ?? []) { + byFlag['']?.delete(name); + byFlag['--dev']?.delete(name); + } - if (conflict) { - try { - const fullImport = imp.path - ? `${imp.packageName}/${imp.path}` - : imp.packageName; - require.resolve(fullImport, { - paths: [localPkg.dir], + for (const [flag, names] of Object.entries(byFlag)) { + // The security implication of this is a bit interesting, as crafted add-import + // directives could be used to install malicious packages. However, the same is true + // for adding malicious packages to package.json, so there's significant difference. + execFileSync('yarn', ['add', ...(flag || []), ...names], { + cwd: localPkg.dir, + stdio: 'inherit', }); - } catch { - // If the dependency doesn't resolve then it's likely a type import, ignore + } + + // This switches all import directives back to the original import. + for (const added of importsToAdd) { + context.report({ + node: added.node, + messageId: 'switchBack', + fix(fixer) { + return fixer.replaceText(added.node, `'${added.name}'`); + }, + }); + } + + importsToAdd.length = 0; + packages.clearCache(); + }, + ...visitImports(context, (node, imp) => { + // We leave checking of type imports to the repo-tools check, + // and we skip builtins and local imports + if ( + imp.kind === 'type' || + imp.type === 'builtin' || + imp.type === 'local' + ) { return; } - const packagePath = path.relative(packages.root.dir, localPkg.dir); - const packageJsonPath = path.join(packagePath, 'package.json'); + // Any import directive that is found is collected for processing later + if (imp.type === 'directive') { + const parts = imp.path.split(':'); + if (parts[1] !== 'add-import') { + return; + } + const [type, name] = parts.slice(2); + if (!name.match(/^(@[-\w\.~]+\/)?[-\w\.~]*$/i)) { + throw new Error( + `Invalid package name to add as dependency: '${name}'`, + ); + } - context.report({ - node, - messageId: conflict.oldDepsField ? 'switch' : 'undeclared', - data: { - ...conflict, - packagePath, - addFlag: getAddFlagForDepsField(conflict.depsField), - packageName: imp.packageName, - packageJsonPath: packageJsonPath, - }, - }); - } - }); + importsToAdd.push({ + flag: getAddFlagForDepsField(type).trim(), + name, + node: imp.node, + }); + return; + } + + // We skip imports for the package itself + if (imp.packageName === localPkg.packageJson.name) { + return; + } + + const modulePath = path.relative(localPkg.dir, filePath); + const expectedType = getExpectedDepType( + localPkg.packageJson, + imp.packageName, + modulePath, + ); + + const conflict = findConflict( + localPkg.packageJson, + imp.packageName, + expectedType, + ); + + if (conflict) { + try { + const fullImport = imp.path + ? `${imp.packageName}/${imp.path}` + : imp.packageName; + require.resolve(fullImport, { + paths: [localPkg.dir], + }); + } catch { + // If the dependency doesn't resolve then it's likely a type import, ignore + return; + } + + const packagePath = path.relative(packages.root.dir, localPkg.dir); + const packageJsonPath = path.join(packagePath, 'package.json'); + + context.report({ + node, + messageId: conflict.oldDepsField ? 'switch' : 'undeclared', + data: { + ...conflict, + packagePath, + addFlag: getAddFlagForDepsField(conflict.depsField), + packageName: imp.packageName, + packageJsonPath: packageJsonPath, + }, + // This fix callback is always executed, regardless of whether linting is run with + // fixes enabled or not. There is no way to determine if fixes are being applied, so + // instead our fix will replace the import with a directive that will be picked up + // on the next run. When ESLint applies fixes all rules are re-run to make sure the fixes + // applied correctly, which means that these directives will be picked up, executed, + // and switched back to the original import immediately. + // This is not true for all editor integrations. For example, VSCode translates there fixes + // to native editor commands, and does not re-run ESLint. This means that the import directive + // will end up in source code, and the import directive fix needs to be applied manually too. + // There is to my knowledge no way around this that doesn't get very hacky, so it will do for now. + fix: conflict.oldDepsField + ? undefined + : fixer => { + return fixer.replaceText( + imp.node, + `'directive:add-import:${conflict.depsField}:${imp.packageName}'`, + ); + }, + }); + } + }), + }; }, }; diff --git a/packages/eslint-plugin/src/no-undeclared-imports.test.ts b/packages/eslint-plugin/src/no-undeclared-imports.test.ts index 78844899ba..e92d422057 100644 --- a/packages/eslint-plugin/src/no-undeclared-imports.test.ts +++ b/packages/eslint-plugin/src/no-undeclared-imports.test.ts @@ -18,6 +18,10 @@ import { RuleTester } from 'eslint'; import { join as joinPath } from 'path'; import rule from '../rules/no-undeclared-imports'; +jest.mock('child_process', () => ({ + execFileSync: jest.fn(), +})); + const RULE = 'no-undeclared-imports'; const FIXTURE = joinPath(__dirname, '__fixtures__/monorepo'); @@ -45,6 +49,9 @@ const ERR_SWITCHED = ( 'package.json', )}.`, }); +const ERR_SWITCH_BACK = () => ({ + message: 'Switch back to import declaration', +}); process.chdir(FIXTURE); @@ -130,6 +137,7 @@ ruleTester.run(RULE, rule, { }, { code: `import 'lodash'`, + output: `import 'directive:add-import:dependencies:lodash'`, filename: joinPath(FIXTURE, 'packages/bar/src/index.ts'), errors: [ ERR_UNDECLARED('lodash', 'dependencies', joinPath('packages', 'bar')), @@ -137,6 +145,7 @@ ruleTester.run(RULE, rule, { }, { code: `import { debounce } from 'lodash'`, + output: `import { debounce } from 'directive:add-import:dependencies:lodash'`, filename: joinPath(FIXTURE, 'packages/bar/src/index.ts'), errors: [ ERR_UNDECLARED('lodash', 'dependencies', joinPath('packages', 'bar')), @@ -144,6 +153,7 @@ ruleTester.run(RULE, rule, { }, { code: `import * as _ from 'lodash'`, + output: `import * as _ from 'directive:add-import:dependencies:lodash'`, filename: joinPath(FIXTURE, 'packages/bar/src/index.ts'), errors: [ ERR_UNDECLARED('lodash', 'dependencies', joinPath('packages', 'bar')), @@ -151,6 +161,7 @@ ruleTester.run(RULE, rule, { }, { code: `import _ from 'lodash'`, + output: `import _ from 'directive:add-import:dependencies:lodash'`, filename: joinPath(FIXTURE, 'packages/bar/src/index.ts'), errors: [ ERR_UNDECLARED('lodash', 'dependencies', joinPath('packages', 'bar')), @@ -158,6 +169,7 @@ ruleTester.run(RULE, rule, { }, { code: `import('lodash')`, + output: `import('directive:add-import:dependencies:lodash')`, filename: joinPath(FIXTURE, 'packages/bar/src/index.ts'), errors: [ ERR_UNDECLARED('lodash', 'dependencies', joinPath('packages', 'bar')), @@ -165,6 +177,7 @@ ruleTester.run(RULE, rule, { }, { code: `require('lodash')`, + output: `require('directive:add-import:dependencies:lodash')`, filename: joinPath(FIXTURE, 'packages/bar/src/index.ts'), errors: [ ERR_UNDECLARED('lodash', 'dependencies', joinPath('packages', 'bar')), @@ -172,13 +185,7 @@ ruleTester.run(RULE, rule, { }, { code: `import 'lodash'`, - filename: joinPath(FIXTURE, 'packages/bar/src/index.ts'), - errors: [ - ERR_UNDECLARED('lodash', 'dependencies', joinPath('packages', 'bar')), - ], - }, - { - code: `import 'lodash'`, + output: `import 'directive:add-import:devDependencies:lodash'`, filename: joinPath(FIXTURE, 'packages/bar/src/index.test.ts'), errors: [ ERR_UNDECLARED( @@ -191,6 +198,7 @@ ruleTester.run(RULE, rule, { }, { code: `import 'react'`, + output: `import 'directive:add-import:peerDependencies:react'`, filename: joinPath(FIXTURE, 'packages/bar/src/index.ts'), errors: [ ERR_UNDECLARED( @@ -203,6 +211,7 @@ ruleTester.run(RULE, rule, { }, { code: `import 'react'`, + output: `import 'directive:add-import:peerDependencies:react'`, filename: joinPath(FIXTURE, 'packages/bar/src/index.test.ts'), errors: [ ERR_UNDECLARED( @@ -215,6 +224,7 @@ ruleTester.run(RULE, rule, { }, { code: `import 'react-dom'`, + output: `import 'directive:add-import:dependencies:react-dom'`, filename: joinPath(FIXTURE, 'packages/foo/src/index.ts'), errors: [ ERR_UNDECLARED( @@ -226,6 +236,7 @@ ruleTester.run(RULE, rule, { }, { code: `import 'react-dom'`, + output: `import 'directive:add-import:devDependencies:react-dom'`, filename: joinPath(FIXTURE, 'packages/foo/src/index.test.ts'), errors: [ ERR_UNDECLARED( @@ -238,6 +249,7 @@ ruleTester.run(RULE, rule, { }, { code: `import '@internal/foo'`, + output: `import 'directive:add-import:dependencies:@internal/foo'`, filename: joinPath(FIXTURE, 'packages/bar/src/index.ts'), errors: [ ERR_UNDECLARED( @@ -247,5 +259,43 @@ ruleTester.run(RULE, rule, { ), ], }, + + // Switching back to original import declarations + { + code: `import 'directive:add-import:dependencies:lodash'`, + output: `import 'lodash'`, + filename: joinPath(FIXTURE, 'packages/bar/src/index.ts'), + errors: [ERR_SWITCH_BACK()], + }, + { + code: `import { debounce } from 'directive:add-import:dependencies:lodash'`, + output: `import { debounce } from 'lodash'`, + filename: joinPath(FIXTURE, 'packages/bar/src/index.ts'), + errors: [ERR_SWITCH_BACK()], + }, + { + code: `import * as _ from 'directive:add-import:dependencies:lodash'`, + output: `import * as _ from 'lodash'`, + filename: joinPath(FIXTURE, 'packages/bar/src/index.ts'), + errors: [ERR_SWITCH_BACK()], + }, + { + code: `import _ from 'directive:add-import:dependencies:lodash'`, + output: `import _ from 'lodash'`, + filename: joinPath(FIXTURE, 'packages/bar/src/index.ts'), + errors: [ERR_SWITCH_BACK()], + }, + { + code: `import('directive:add-import:dependencies:lodash')`, + output: `import('lodash')`, + filename: joinPath(FIXTURE, 'packages/bar/src/index.ts'), + errors: [ERR_SWITCH_BACK()], + }, + { + code: `require('directive:add-import:dependencies:lodash')`, + output: `require('lodash')`, + filename: joinPath(FIXTURE, 'packages/bar/src/index.ts'), + errors: [ERR_SWITCH_BACK()], + }, ], });