From 4d05d5bf8362ac5e86c6e75918aa7cdbb14a3a79 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 3 Feb 2023 11:10:38 +0100 Subject: [PATCH] eslint-plugin: refactor to ES2021 and add no-undeclared-imports rule Signed-off-by: Patrik Oldsberg --- packages/eslint-plugin/.eslintrc.js | 7 +- packages/eslint-plugin/index.js | 2 + packages/eslint-plugin/lib/getPackages.js | 63 +++++++ packages/eslint-plugin/lib/visitImports.js | 100 +++++++++++ packages/eslint-plugin/package.json | 3 +- .../rules/no-forbidden-package-imports.js | 91 ++-------- .../rules/no-undeclared-imports.js | 156 ++++++++++++++++++ yarn.lock | 1 + 8 files changed, 339 insertions(+), 84 deletions(-) create mode 100644 packages/eslint-plugin/lib/getPackages.js create mode 100644 packages/eslint-plugin/lib/visitImports.js create mode 100644 packages/eslint-plugin/rules/no-undeclared-imports.js diff --git a/packages/eslint-plugin/.eslintrc.js b/packages/eslint-plugin/.eslintrc.js index aaf907cfbc..74f93a6475 100644 --- a/packages/eslint-plugin/.eslintrc.js +++ b/packages/eslint-plugin/.eslintrc.js @@ -17,8 +17,9 @@ // Custom lint config here since source is ES5 module.exports = { plugins: ['import'], - parserOptions: { - ecmaVersion: 5, + env: { + node: true, + es2021: true, }, rules: { 'import/no-extraneous-dependencies': [ @@ -30,6 +31,6 @@ module.exports = { bundledDependencies: true, }, ], - 'no-unused-expressions': 'off', + // 'no-unused-expressions': 'off', }, }; diff --git a/packages/eslint-plugin/index.js b/packages/eslint-plugin/index.js index 11d17b2b8f..cf1dd20ab7 100644 --- a/packages/eslint-plugin/index.js +++ b/packages/eslint-plugin/index.js @@ -20,10 +20,12 @@ module.exports = { plugins: ['@backstage'], rules: { '@backstage/no-forbidden-package-imports': 'error', + '@backstage/no-undeclared-imports': 'error', }, }, }, rules: { 'no-forbidden-package-imports': require('./rules/no-forbidden-package-imports'), + 'no-undeclared-imports': require('./rules/no-undeclared-imports'), }, }; diff --git a/packages/eslint-plugin/lib/getPackages.js b/packages/eslint-plugin/lib/getPackages.js new file mode 100644 index 0000000000..91043fe0d5 --- /dev/null +++ b/packages/eslint-plugin/lib/getPackages.js @@ -0,0 +1,63 @@ +/* + * Copyright 2023 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// @ts-check + +/** @type {import('@manypkg/get-packages')} */ +const manypkg = require('@manypkg/get-packages'); + +/** + * @typedef ExtendedPackage + * @type {import('@manypkg/get-packages').Package & { packageJson: { exports?: Record }}} packageJson + */ + +/** + * @typedef PackageMap + * @type object + * + * @property {ExtendedPackage} root + * @property {ExtendedPackage[]} list + * @property {Map} map + */ + +// Loads all packages in the monorepo once, and caches the result +module.exports = (function () { + let result = undefined; + let lastLoadAt = undefined; + + /** @returns {PackageMap | undefined} */ + return function getPackages(/** @type {string} */ dir) { + if (result) { + // Only cache for 5 seconds, to avoid the need to reload ESLint servers + if (Date.now() - lastLoadAt > 5000) { + result = undefined; + } else { + return result; + } + } + const packages = manypkg.getPackagesSync(dir); + if (!packages) { + return undefined; + } + result = { + map: new Map(packages.packages.map(pkg => [pkg.packageJson.name, pkg])), + list: packages.packages, + root: packages.root, + }; + lastLoadAt = Date.now(); + return result; + }; +})(); diff --git a/packages/eslint-plugin/lib/visitImports.js b/packages/eslint-plugin/lib/visitImports.js new file mode 100644 index 0000000000..d73c3203a8 --- /dev/null +++ b/packages/eslint-plugin/lib/visitImports.js @@ -0,0 +1,100 @@ +/* + * Copyright 2023 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// @ts-check + +const getPackages = require('./getPackages'); + +/** + * @typedef LocalImport + * @type {object} + * @property {'local'} type + * @property {string} path + */ + +/** + * @typedef InternalImport + * @type {object} + * @property {'internal'} type + * @property {string} path + * @property {import('./getPackages').ExtendedPackage} package + */ + +/** + * @typedef ExternalImport + * @type {object} + * @property {'external'} type + * @property {string} path + * @property {string} packageName + */ + +/** + * @callback ImportVisitor + * @param {import('eslint').Rule.Node} node + * @param {LocalImport | InternalImport | ExternalImport} import + */ + +/** + * @param visitor - Visitor callback + * @param {import('eslint').Rule.RuleContext} context + * @param {ImportVisitor} visitor + */ +module.exports = function visitImports(context, visitor) { + const packages = getPackages(context.getCwd()); + if (!packages) { + return; + } + + function visit(node) { + if (!node.source) { + return; + } + const importPath = node.source.value; + if (importPath[0] === '.') { + return visitor(node, { type: 'local', path: importPath }); + } + + const pathParts = importPath.split('/'); + + // Check for match with plain name, then namespaced name + let packageName; + let subPath; + if (importPath[0] === '@') { + packageName = pathParts.slice(0, 2).join('/'); + subPath = pathParts.slice(2).join('/'); + } else { + packageName = pathParts[0]; + subPath = pathParts.slice(1).join('/'); + } + const pkg = packages?.map.get(packageName); + if (!pkg) { + return visitor(node, { + type: 'external', + path: subPath, + packageName: packageName, + }); + } + + return visitor(node, { type: 'internal', path: subPath, package: pkg }); + } + + return { + ImportDeclaration: visit, + ExportAllDeclaration: visit, + ExportNamedDeclaration: visit, + ImportExpression: visit, + }; +}; diff --git a/packages/eslint-plugin/package.json b/packages/eslint-plugin/package.json index ee5e62c41e..73862d2295 100644 --- a/packages/eslint-plugin/package.json +++ b/packages/eslint-plugin/package.json @@ -17,7 +17,8 @@ "lint": "backstage-cli package lint" }, "dependencies": { - "@manypkg/get-packages": "^1.1.3" + "@manypkg/get-packages": "^1.1.3", + "minimatch": "^5.1.2" }, "devDependencies": { "@backstage/cli": "workspace:^" diff --git a/packages/eslint-plugin/rules/no-forbidden-package-imports.js b/packages/eslint-plugin/rules/no-forbidden-package-imports.js index d5cce80368..bbf342dba0 100644 --- a/packages/eslint-plugin/rules/no-forbidden-package-imports.js +++ b/packages/eslint-plugin/rules/no-forbidden-package-imports.js @@ -1,4 +1,3 @@ -'use strict'; /* * Copyright 2023 The Backstage Authors * @@ -17,41 +16,9 @@ // @ts-check -// Note: this file is ES5, because we want to avoid having to transpile it for local development - -var fs = require('fs'); -var path = require('path'); -/** @type {import('@manypkg/get-packages')} */ -var manypkg = require('@manypkg/get-packages'); - -// Loads all packages in the monorepo once, and caches the result -var getPackageMap = (function () { - var map = undefined; - var lastLoadAt = undefined; - - /** @returns {Record} */ - return function (/** @type {string} */ dir) { - if (map) { - // Only cache for 5 seconds, to avoid the need to reload ESLint servers - if (Date.now() - lastLoadAt > 5000) { - map = undefined; - } else { - return map; - } - } - var packages = manypkg.getPackagesSync(dir).packages; - if (!packages) { - return undefined; - } - map = {}; - lastLoadAt = Date.now(); - packages.forEach(function (pkg) { - map[pkg.packageJson.name] = pkg; - }); - return map; - }; -})(); +const visitImports = require('../lib/visitImports'); +/** @type {import('eslint').Rule.RuleModule} */ module.exports = { meta: { type: 'problem', @@ -59,48 +26,19 @@ module.exports = { forbidden: '{{packageName}} does not export {{subPath}}', }, }, - create: function (/** @type import('eslint').Rule.RuleContext */ context) { - var packageMap = getPackageMap(context.getCwd()); - if (!packageMap) { - return; - } - - function checkImport(node) { - if (!node.source) { + create(context) { + return visitImports(context, (node, imp) => { + if (imp.type !== 'internal') { return; } - var importPath = node.source.value; - if (importPath[0] === '.') { - return; - } - - var pathParts = importPath.split('/'); - - // Check for match with plain name, then namespaced name - var name = pathParts.shift(); - if (!packageMap[name]) { - name += '/' + pathParts.shift(); - } - if (!packageMap[name]) { - return; - } - // Empty subpaths are always allowed - var subPath = pathParts.join('/'); - if (!subPath) { + if (!imp.path) { return; } - var pkg = packageMap[name]; - // If the import is listed in the package.json exports field, we allow it - var exp = pkg.packageJson.exports; - if (exp && (exp[subPath] || exp['./' + subPath])) { - return; - } - - // If there's a package.json in the target directory, we allow the import - if (fs.existsSync(path.resolve(pkg.dir, subPath, 'package.json'))) { + const exp = imp.package.packageJson.exports; + if (exp && (exp[imp.path] || exp['./' + imp.path])) { return; } @@ -108,17 +46,10 @@ module.exports = { node: node, messageId: 'forbidden', data: { - packageName: pkg.packageJson.name || '', - subPath: subPath, + packageName: imp.package.packageJson.name || imp.package.dir, + subPath: imp.path, }, }); - } - - return { - ImportDeclaration: checkImport, - ExportAllDeclaration: checkImport, - ExportNamedDeclaration: checkImport, - ImportExpression: checkImport, - }; + }); }, }; diff --git a/packages/eslint-plugin/rules/no-undeclared-imports.js b/packages/eslint-plugin/rules/no-undeclared-imports.js new file mode 100644 index 0000000000..462b4b2e31 --- /dev/null +++ b/packages/eslint-plugin/rules/no-undeclared-imports.js @@ -0,0 +1,156 @@ +/* + * Copyright 2023 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// @ts-check + +const path = require('path'); +const getPackageMap = require('../lib/getPackages'); +const visitImports = require('../lib/visitImports'); +const minimatch = require('minimatch'); + +const depFields = { + dep: 'dependencies', + dev: 'devDependencies', + peer: 'peerDependencies', +}; + +const peerDeps = ['react', 'react-dom', 'react-router', 'react-router-dom']; +const devModulePatterns = [ + new minimatch.Minimatch('!src/**'), + new minimatch.Minimatch('src/**/*.test.*'), + new minimatch.Minimatch('src/**/*.stories.*'), + new minimatch.Minimatch('src/**/__testUtils__/**'), + new minimatch.Minimatch('src/**/__mocks__/**'), + new minimatch.Minimatch('src/setupTests.*'), +]; + +function getExpectedDepType( + /** @type {string} */ impPath, + /** @type {string} */ modulePath, +) { + if (peerDeps.includes(impPath)) { + return 'peer'; + } + for (const pattern of devModulePatterns) { + if (pattern.match(modulePath)) { + return 'dev'; + } + } + return 'dep'; +} + +/** + * + * @param {import('@manypkg/get-packages').Package['packageJson']} pkg + * @param {string} name + * @param {ReturnType} expectedType + * @returns {{oldDepsField?: string, depsField: string} | undefined} + */ +function findConflict(pkg, name, expectedType) { + const isDep = pkg.dependencies?.[name]; + const isDevDep = pkg.devDependencies?.[name]; + const isPeerDep = pkg.peerDependencies?.[name]; + const depsField = depFields[expectedType]; + + if (expectedType === 'dep' && !isDep) { + const oldDepsField = isDevDep ? depFields.dev : undefined; + return { oldDepsField, depsField }; + } else if (expectedType === 'dev' && !isDevDep && !isDep && !isPeerDep) { + return { oldDepsField: undefined, depsField }; + } else if (expectedType === 'peer' && !isPeerDep) { + const oldDepsField = isDep + ? depFields.dep + : isDevDep + ? depFields.dev + : undefined; + + return { oldDepsField, depsField }; + } +} + +function getAddFlagForConflict(conflict) { + switch (conflict?.depsField) { + case depFields.dep: + return ''; + case depFields.dev: + return ' --dev'; + case depFields.peer: + return ' --peer'; + default: + return ''; + } +} + +/** @type {import('eslint').Rule.RuleModule} */ +module.exports = { + meta: { + type: 'problem', + 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 }}.', + }, + }, + create(context) { + const packages = getPackageMap(context.getCwd()); + if (!packages) { + return {}; + } + const filePath = context.getPhysicalFilename + ? context.getPhysicalFilename() + : context.getFilename(); + + const localPkg = packages.list.find(p => + filePath.startsWith(p.dir + path.sep), + ); + if (!localPkg) { + return {}; + } + + return visitImports(context, (node, imp) => { + if (imp.type !== 'external') { + return; + } + + const modulePath = path.relative(localPkg.dir, filePath); + const expectedType = getExpectedDepType(imp.packageName, modulePath); + + const conflict = findConflict( + localPkg.packageJson, + imp.packageName, + expectedType, + ); + + if (conflict) { + 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: getAddFlagForConflict(conflict), + packageName: imp.packageName, + packageJsonPath: packageJsonPath, + }, + }); + } + }); + }, +}; diff --git a/yarn.lock b/yarn.lock index 06cd22d84a..e0e1da349c 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4101,6 +4101,7 @@ __metadata: dependencies: "@backstage/cli": "workspace:^" "@manypkg/get-packages": ^1.1.3 + minimatch: ^5.1.2 languageName: unknown linkType: soft