From 08895e3f84aec81f6391e990ef7a53032fbe45b3 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 4 Sep 2024 09:56:39 +0300 Subject: [PATCH] eslint-plugin: add support for inline packages Signed-off-by: Patrik Oldsberg --- .changeset/thirty-bottles-rest.md | 5 + packages/eslint-plugin/lib/getPackages.js | 2 +- .../rules/no-undeclared-imports.js | 304 ++++++++++++++---- .../monorepo/packages/bar/package.json | 7 +- .../inline-dep-invalid-direct/package.json | 12 + .../inline-dep-invalid-missing/package.json | 7 + .../packages/inline-dep-valid/package.json | 10 + .../monorepo/packages/inline/package.json | 16 + .../src/no-undeclared-imports.test.ts | 49 +++ 9 files changed, 350 insertions(+), 62 deletions(-) create mode 100644 .changeset/thirty-bottles-rest.md create mode 100644 packages/eslint-plugin/src/__fixtures__/monorepo/packages/inline-dep-invalid-direct/package.json create mode 100644 packages/eslint-plugin/src/__fixtures__/monorepo/packages/inline-dep-invalid-missing/package.json create mode 100644 packages/eslint-plugin/src/__fixtures__/monorepo/packages/inline-dep-valid/package.json create mode 100644 packages/eslint-plugin/src/__fixtures__/monorepo/packages/inline/package.json diff --git a/.changeset/thirty-bottles-rest.md b/.changeset/thirty-bottles-rest.md new file mode 100644 index 0000000000..51b572d507 --- /dev/null +++ b/.changeset/thirty-bottles-rest.md @@ -0,0 +1,5 @@ +--- +'@backstage/eslint-plugin': patch +--- + +Added support for linting dependencies on workspace packages with the `backstage.inline` flag. diff --git a/packages/eslint-plugin/lib/getPackages.js b/packages/eslint-plugin/lib/getPackages.js index 8f4ccfacf3..3b46f41a54 100644 --- a/packages/eslint-plugin/lib/getPackages.js +++ b/packages/eslint-plugin/lib/getPackages.js @@ -21,7 +21,7 @@ const manypkg = require('@manypkg/get-packages'); /** * @typedef ExtendedPackage - * @type {import('@manypkg/get-packages').Package & { packageJson: { exports?: Record, files?: Array }}} packageJson + * @type {import('@manypkg/get-packages').Package & { packageJson: { exports?: Record, files?: Array, backstage?: { inline?: boolean } }}} packageJson */ /** diff --git a/packages/eslint-plugin/rules/no-undeclared-imports.js b/packages/eslint-plugin/rules/no-undeclared-imports.js index d2c92ccb8a..0b6a4289b5 100644 --- a/packages/eslint-plugin/rules/no-undeclared-imports.js +++ b/packages/eslint-plugin/rules/no-undeclared-imports.js @@ -22,11 +22,11 @@ const visitImports = require('../lib/visitImports'); const minimatch = require('minimatch'); const { execFileSync } = require('child_process'); -const depFields = { +const depFields = /** @type {const} */ ({ dep: 'dependencies', dev: 'devDependencies', peer: 'peerDependencies', -}; +}); const devModulePatterns = [ new minimatch.Minimatch('!src/**'), @@ -154,6 +154,124 @@ function addVersionQuery(name, flag, packages) { return `${name}@${mostCommonRange}`; } +/** + * Add missing package imports + * @param {Array<{name: string, flag: string, node: import('estree').Node}>} toAdd + * @param {import('../lib/getPackages').PackageMap} packages + * @param {import('../lib/getPackages').ExtendedPackage} localPkg + */ +function addMissingImports(toAdd, packages, localPkg) { + /** @type Record> */ + const byFlag = {}; + + for (const { name, flag } of toAdd) { + byFlag[flag] = byFlag[flag] ?? new Set(); + byFlag[flag].add(name); + } + + for (const name of byFlag[''] ?? []) { + byFlag['--dev']?.delete(name); + } + for (const name of byFlag['--peer'] ?? []) { + byFlag['']?.delete(name); + byFlag['--dev']?.delete(name); + } + + for (const [flag, names] of Object.entries(byFlag)) { + // Look up existing version queries in the repo for the same dependency + const namesWithQuery = [...names].map(name => + addVersionQuery(name, flag, packages), + ); + + // 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 no significant difference. + execFileSync('yarn', ['add', ...(flag ? [flag] : []), ...namesWithQuery], { + cwd: localPkg.dir, + stdio: 'inherit', + }); + } +} + +/** + * Removes dependency entries pointing to inlined workspace packages. + * @param {Array<{pkg: import('../lib/getPackages').ExtendedPackage, node: import('estree').Node}>} toInline + * @param {import('../lib/getPackages').ExtendedPackage} localPkg + */ +function removeInlineImports(toInline, localPkg) { + /** @type Set */ + const toRemove = new Set(); + + for (const { pkg } of toInline) { + const name = pkg.packageJson.name; + for (const depType of Object.values(depFields)) { + if (localPkg.packageJson[depType]?.[name]) { + toRemove.add(name); + } + } + } + if (toRemove.size > 0) { + execFileSync('yarn', ['remove', ...toRemove], { + cwd: localPkg.dir, + stdio: 'inherit', + }); + } +} + +/** + * Adds dependencies that are not properly forwarded from inline dependencies. + * @param {Array<{pkg: import('../lib/getPackages').ExtendedPackage, node: import('estree').Node}>} toInline + * @param {import('../lib/getPackages').ExtendedPackage} localPkg + */ +function addForwardedInlineImports(toInline, localPkg) { + const declaredProdDeps = new Set([ + ...Object.keys(localPkg.packageJson.dependencies ?? {}), + ...Object.keys(localPkg.packageJson.peerDependencies ?? {}), + localPkg.packageJson.name, // include self + ]); + + /** @type Map> */ + const byFlagByName = new Map(); + + for (const { pkg } of toInline) { + for (const depType of /** @type {const} */ ([ + 'dependencies', + 'peerDependencies', + ])) { + for (const [depName, depQuery] of Object.entries( + pkg.packageJson[depType] ?? {}, + )) { + if (!declaredProdDeps.has(depName)) { + const flag = getAddFlagForDepsField(depType); + const byName = byFlagByName.get(flag); + if (byName) { + const query = byName.get(depName); + if (query && query !== depQuery) { + throw new Error( + `Conflicting dependency queries for inlined package dep ${depName}, got ${query} and ${depQuery}`, + ); + } else { + byName.set(depName, depQuery); + } + } else { + byFlagByName.set(flag, new Map([[depName, depQuery]])); + } + } + } + } + } + + for (const [flag, byName] of byFlagByName) { + const namesWithQuery = [...byName.entries()].map( + ([name, query]) => `${name}@${query}`, + ); + execFileSync('yarn', ['add', ...(flag ? [flag] : []), ...namesWithQuery], { + cwd: localPkg.dir, + stdio: 'inherit', + }); + } +} + /** @type {import('eslint').Rule.RuleModule} */ module.exports = { meta: { @@ -165,6 +283,8 @@ module.exports = { switch: '{{ packageName }} is declared in {{ oldDepsField }}, but should be moved to {{ depsField }} in {{ packageJsonPath }}.', switchBack: 'Switch back to import declaration', + inlineDirect: `The dependency on the inline package {{ packageName }} must not be declared in package dependencies.`, + inlineMissing: `Each production dependency from the inline package {{ packageName }} must be re-declared by this package, the following dependencies are missing: {{ missingDeps }}`, }, docs: { description: @@ -189,57 +309,48 @@ module.exports = { /** @type Array<{name: string, flag: string, node: import('estree').Node}> */ const importsToAdd = []; + /** @type Array<{pkg: import('../lib/getPackages').ExtendedPackage, node: import('estree').Node}> */ + const importsToInline = []; + 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 = {}; + if (importsToAdd.length > 0) { + addMissingImports(importsToAdd, packages, localPkg); - for (const { name, flag } of importsToAdd) { - byFlag[flag] = byFlag[flag] ?? new Set(); - byFlag[flag].add(name); + // 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; } - for (const name of byFlag[''] ?? []) { - byFlag['--dev']?.delete(name); - } - for (const name of byFlag['--peer'] ?? []) { - byFlag['']?.delete(name); - byFlag['--dev']?.delete(name); + if (importsToInline.length > 0) { + removeInlineImports(importsToInline, localPkg); + addForwardedInlineImports(importsToInline, localPkg); + + for (const inlined of importsToInline) { + context.report({ + node: inlined.node, + messageId: 'switchBack', + fix(fixer) { + return fixer.replaceText( + inlined.node, + `'${inlined.pkg.packageJson.name}'`, + ); + }, + }); + } + importsToInline.length = 0; } - for (const [flag, names] of Object.entries(byFlag)) { - // Look up existing version queries in the repo for the same dependency - const namesWithQuery = [...names].map(name => - addVersionQuery(name, flag, packages), - ); - - // 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 no significant difference. - execFileSync( - 'yarn', - ['add', ...(flag ? [flag] : []), ...namesWithQuery], - { - cwd: localPkg.dir, - stdio: 'inherit', - }, - ); - } - - // 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) => { @@ -255,22 +366,99 @@ module.exports = { // 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}'`, - ); + const [, directive, ...args] = imp.path.split(':'); + + if (directive === 'add-import') { + const [type, name] = args; + if (!name.match(/^(@[-\w\.~]+\/)?[-\w\.~]*$/i)) { + throw new Error( + `Invalid package name to add as dependency: '${name}'`, + ); + } + + importsToAdd.push({ + flag: getAddFlagForDepsField(type).trim(), + name, + node: imp.node, + }); + } + + if (directive === 'inline-imports') { + const [name] = args; + const pkg = packages.map.get(name); + if (!pkg) { + throw new Error(`Unexpectedly missing inline package: ${name}`); + } + + importsToInline.push({ + pkg: pkg, + node: imp.node, + }); + } + + return; + } + + // Importing an internal inlined package, which that imports are inlined too + if ( + imp.type === 'internal' && + imp.package.packageJson.backstage?.inline + ) { + for (const depType of Object.values(depFields)) { + if (localPkg.packageJson[depType]?.[imp.packageName]) { + context.report({ + node, + messageId: 'inlineDirect', + data: { + packageName: imp.packageName, + }, + fix: fixer => { + return fixer.replaceText( + imp.node, + `'directive:inline-imports:${imp.packageName}'`, + ); + }, + }); + return; + } + } + + const missingDeps = []; + const declaredProdDeps = new Set([ + ...Object.keys(localPkg.packageJson.dependencies ?? {}), + ...Object.keys(localPkg.packageJson.peerDependencies ?? {}), + localPkg.packageJson.name, // include self + ]); + for (const depType of /** @type {const} */ ([ + 'dependencies', + 'peerDependencies', + ])) { + for (const depName of Object.keys( + imp.package.packageJson[depType] ?? {}, + )) { + if (!declaredProdDeps.has(depName)) { + missingDeps.push(depName); + } + } + } + + if (missingDeps.length > 0) { + context.report({ + node, + messageId: 'inlineMissing', + data: { + packageName: imp.packageName, + missingDeps: missingDeps.join(', '), + }, + fix: fixer => { + return fixer.replaceText( + imp.node, + `'directive:inline-imports:${imp.packageName}'`, + ); + }, + }); } - importsToAdd.push({ - flag: getAddFlagForDepsField(type).trim(), - name, - node: imp.node, - }); return; } diff --git a/packages/eslint-plugin/src/__fixtures__/monorepo/packages/bar/package.json b/packages/eslint-plugin/src/__fixtures__/monorepo/packages/bar/package.json index efef27047b..ba55598dc2 100644 --- a/packages/eslint-plugin/src/__fixtures__/monorepo/packages/bar/package.json +++ b/packages/eslint-plugin/src/__fixtures__/monorepo/packages/bar/package.json @@ -1,14 +1,15 @@ { "name": "@internal/bar", + "backstage": { + "role": "frontend-plugin" + }, "exports": { ".": "./src/index.ts", "./BarPage": "./src/components/Bar.tsx", "./package.json": "./package.json" }, - "backstage": { - "role": "frontend-plugin" - }, "dependencies": { + "inline-dep": "*", "react-router": "*" }, "devDependencies": { diff --git a/packages/eslint-plugin/src/__fixtures__/monorepo/packages/inline-dep-invalid-direct/package.json b/packages/eslint-plugin/src/__fixtures__/monorepo/packages/inline-dep-invalid-direct/package.json new file mode 100644 index 0000000000..352a02d66c --- /dev/null +++ b/packages/eslint-plugin/src/__fixtures__/monorepo/packages/inline-dep-invalid-direct/package.json @@ -0,0 +1,12 @@ +{ + "name": "@internal/inline-dep-direct", + "files": [ + "dist", + "type-utils" + ], + "dependencies": { + "@internal/inline": "workspace:^", + "@internal/inline-dep-valid": "workspace:^", + "react": "*" + } +} diff --git a/packages/eslint-plugin/src/__fixtures__/monorepo/packages/inline-dep-invalid-missing/package.json b/packages/eslint-plugin/src/__fixtures__/monorepo/packages/inline-dep-invalid-missing/package.json new file mode 100644 index 0000000000..595ee4a86b --- /dev/null +++ b/packages/eslint-plugin/src/__fixtures__/monorepo/packages/inline-dep-invalid-missing/package.json @@ -0,0 +1,7 @@ +{ + "name": "@internal/inline-dep-missing", + "files": [ + "dist", + "type-utils" + ] +} diff --git a/packages/eslint-plugin/src/__fixtures__/monorepo/packages/inline-dep-valid/package.json b/packages/eslint-plugin/src/__fixtures__/monorepo/packages/inline-dep-valid/package.json new file mode 100644 index 0000000000..cbb42d0080 --- /dev/null +++ b/packages/eslint-plugin/src/__fixtures__/monorepo/packages/inline-dep-valid/package.json @@ -0,0 +1,10 @@ +{ + "name": "@internal/inline-dep-valid", + "files": [ + "dist", + "type-utils" + ], + "peerDependencies": { + "react": "*" + } +} diff --git a/packages/eslint-plugin/src/__fixtures__/monorepo/packages/inline/package.json b/packages/eslint-plugin/src/__fixtures__/monorepo/packages/inline/package.json new file mode 100644 index 0000000000..fb9345f140 --- /dev/null +++ b/packages/eslint-plugin/src/__fixtures__/monorepo/packages/inline/package.json @@ -0,0 +1,16 @@ +{ + "name": "@internal/inline", + "backstage": { + "inline": true + }, + "files": [ + "dist", + "type-utils" + ], + "dependencies": { + "@internal/inline-dep-valid": "workspace:^" + }, + "peerDependencies": { + "react": "*" + } +} diff --git a/packages/eslint-plugin/src/no-undeclared-imports.test.ts b/packages/eslint-plugin/src/no-undeclared-imports.test.ts index 29e2c3a87c..ee56c34926 100644 --- a/packages/eslint-plugin/src/no-undeclared-imports.test.ts +++ b/packages/eslint-plugin/src/no-undeclared-imports.test.ts @@ -52,6 +52,12 @@ const ERR_SWITCHED = ( const ERR_SWITCH_BACK = () => ({ message: 'Switch back to import declaration', }); +const ERR_INLINE_DIRECT = (name: string) => ({ + message: `The dependency on the inline package ${name} must not be declared in package dependencies.`, +}); +const ERR_INLINE_MISSING = (name: string, missing: string) => ({ + message: `Each production dependency from the inline package ${name} must be re-declared by this package, the following dependencies are missing: ${missing}`, +}); // cwd must be restored const origDir = process.cwd(); @@ -102,6 +108,17 @@ ruleTester.run(RULE, rule, { code: `require('lod' + 'ash')`, filename: joinPath(FIXTURE, 'packages/bar/src/index.ts'), }, + { + code: `import '@internal/inline'`, + filename: joinPath(FIXTURE, 'packages/inline-dep-valid/src/index.ts'), + }, + { + code: `import '@internal/inline'`, + filename: joinPath( + FIXTURE, + 'packages/inline-dep-valid/src/index.test.ts', + ), + }, ], invalid: [ { @@ -264,6 +281,29 @@ ruleTester.run(RULE, rule, { ), ], }, + { + code: `import '@internal/inline'`, + output: `import 'directive:inline-imports:@internal/inline'`, + filename: joinPath( + FIXTURE, + 'packages/inline-dep-invalid-direct/src/index.ts', + ), + errors: [ERR_INLINE_DIRECT('@internal/inline')], + }, + { + code: `import '@internal/inline'`, + output: `import 'directive:inline-imports:@internal/inline'`, + filename: joinPath( + FIXTURE, + 'packages/inline-dep-invalid-missing/src/index.ts', + ), + errors: [ + ERR_INLINE_MISSING( + '@internal/inline', + '@internal/inline-dep-valid, react', + ), + ], + }, // Switching back to original import declarations { @@ -302,5 +342,14 @@ ruleTester.run(RULE, rule, { filename: joinPath(FIXTURE, 'packages/bar/src/index.ts'), errors: [ERR_SWITCH_BACK()], }, + { + code: `import 'directive:inline-imports:@internal/inline'`, + output: `import '@internal/inline'`, + filename: joinPath( + FIXTURE, + 'packages/inline-dep-invalid-direct/src/index.ts', + ), + errors: [ERR_SWITCH_BACK()], + }, ], });