From 5217c3385fd4cc4b15fc8d033a16d5deeae43e1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Leosvel=20P=C3=A9rez=20Espinosa?= Date: Mon, 22 Jul 2024 16:03:19 +0200 Subject: [PATCH] fix(linter): convert root projects correctly to inferred and remove default option values (#27035) ## Current Behavior Converting a root project to inferred results in the plugin registration to have the `includes` option set to `./**/*`. This is invalid and causes no project to be inferred. Additionally, the eslint `convert-to-inferred` generator: - keeps a redundant `config` option, which is not needed because inferred tasks only work with default/known ESLint config files, so there's no need to specify it in the options - converts all `lintFilePatterns` to `args`, which is correct, but it keeps the patterns that are already inferred by the plugin, which leads to duplicated patterns when running the task ## Expected Behavior Converting a root project to inferred should work as expected and result in the `lint` task being inferred for the project. Also, default inferred options should be removed from the target options. ## Related Issue(s) Fixes #26775 --- .../executor-to-plugin-migrator.ts | 36 +-- .../convert-to-inferred.spec.ts | 253 +++++++++++++++++- .../convert-to-inferred.ts | 75 ++++-- 3 files changed, 315 insertions(+), 49 deletions(-) diff --git a/packages/devkit/src/generators/plugin-migrations/executor-to-plugin-migrator.ts b/packages/devkit/src/generators/plugin-migrations/executor-to-plugin-migrator.ts index 3ffdf037ee818..e314b1e7ca015 100644 --- a/packages/devkit/src/generators/plugin-migrations/executor-to-plugin-migrator.ts +++ b/packages/devkit/src/generators/plugin-migrations/executor-to-plugin-migrator.ts @@ -1,5 +1,6 @@ import { minimatch } from 'minimatch'; import { deepStrictEqual } from 'node:assert'; +import { join } from 'node:path/posix'; import type { InputDefinition, ProjectConfiguration, @@ -39,7 +40,8 @@ type PostTargetTransformer = ( inferredTargetConfiguration: InferredTargetConfiguration ) => TargetConfiguration | Promise; type SkipTargetFilter = ( - targetConfiguration: TargetConfiguration + targetOptions: Record, + projectConfiguration: ProjectConfiguration ) => false | string; type SkipProjectFilter = ( projectConfiguration: ProjectConfiguration @@ -58,7 +60,6 @@ class ExecutorToPluginMigrator { #nxJson: NxJsonConfiguration; #targetDefaultsForExecutor: Partial; #targetAndProjectsToMigrate: Map>; - #pluginToAddForTarget: Map>; #createNodes?: CreateNodes; #createNodesV2?: CreateNodesV2; #createNodesResultsForTargets: Map; @@ -108,7 +109,6 @@ class ExecutorToPluginMigrator { nxJson.plugins ??= []; this.#nxJson = nxJson; this.#targetAndProjectsToMigrate = new Map(); - this.#pluginToAddForTarget = new Map(); this.#createNodesResultsForTargets = new Map(); this.#skippedProjects = new Set(); @@ -118,18 +118,11 @@ class ExecutorToPluginMigrator { } async #migrateTarget(targetName: string) { - const include: string[] = []; for (const projectName of this.#targetAndProjectsToMigrate.get( targetName )) { - include.push(await this.#migrateProject(projectName, targetName)); + await this.#migrateProject(projectName, targetName); } - - this.#pluginToAddForTarget.set(targetName, { - plugin: this.#pluginPath, - options: this.#pluginOptionsBuilder(targetName), - include, - }); } async #migrateProject(projectName: string, targetName: string) { @@ -180,8 +173,6 @@ class ExecutorToPluginMigrator { } updateProjectConfiguration(this.tree, projectName, projectConfig); - - return `${projectFromGraph.data.root}/**/*`; } #mergeInputs( @@ -237,7 +228,12 @@ class ExecutorToPluginMigrator { forEachExecutorOptions( this.tree, this.#executor, - (targetConfiguration, projectName, targetName, configurationName) => { + ( + options: Record, + projectName, + targetName, + configurationName + ) => { if (this.#skippedProjects.has(projectName) || configurationName) { return; } @@ -263,9 +259,12 @@ class ExecutorToPluginMigrator { return; } - const skipTargetReason = this.#skipTargetFilter(targetConfiguration); + const skipTargetReason = this.#skipTargetFilter( + options, + this.#projectGraph.nodes[projectName].data + ); if (skipTargetReason) { - const errorMsg = `${targetName} target on project "${projectName}" cannot be migrated. ${skipTargetReason}`; + const errorMsg = `The ${targetName} target on project "${projectName}" cannot be migrated. ${skipTargetReason}`; if (this.#specificProjectToMigrate) { throw new Error(errorMsg); } else { @@ -538,7 +537,10 @@ async function addPluginRegistrations( ) ); - const projectIncludeGlob = `${projectGraph.nodes[project].data.root}/**/*`; + const projectIncludeGlob = + projectGraph.nodes[project].data.root === '.' + ? '*' + : join(projectGraph.nodes[project].data.root, '**/*'); if (!existingPlugin) { nxJson.plugins ??= []; const plugin: ExpandedPluginConfiguration = { diff --git a/packages/eslint/src/generators/convert-to-inferred/convert-to-inferred.spec.ts b/packages/eslint/src/generators/convert-to-inferred/convert-to-inferred.spec.ts index f1bb20f0deca1..a482c80ce9d78 100644 --- a/packages/eslint/src/generators/convert-to-inferred/convert-to-inferred.spec.ts +++ b/packages/eslint/src/generators/convert-to-inferred/convert-to-inferred.spec.ts @@ -1,24 +1,24 @@ -import { - getRelativeProjectJsonSchemaPath, - updateProjectConfiguration, -} from 'nx/src/generators/utils/project-configuration'; -import { createTreeWithEmptyWorkspace } from '@nx/devkit/testing'; -import { convertToInferred } from './convert-to-inferred'; import { addProjectConfiguration as _addProjectConfiguration, - type ExpandedPluginConfiguration, joinPathFragments, - type ProjectConfiguration, - type ProjectGraph, readJson, readNxJson, readProjectConfiguration, - type Tree, updateNxJson, writeJson, + type ExpandedPluginConfiguration, + type ProjectConfiguration, + type ProjectGraph, + type Tree, } from '@nx/devkit'; import { TempFs } from '@nx/devkit/internal-testing-utils'; +import { createTreeWithEmptyWorkspace } from '@nx/devkit/testing'; import { join } from 'node:path'; +import { + getRelativeProjectJsonSchemaPath, + updateProjectConfiguration, +} from 'nx/src/generators/utils/project-configuration'; +import { convertToInferred } from './convert-to-inferred'; let fs: TempFs; @@ -93,6 +93,7 @@ interface CreateEslintLintProjectOptions { appRoot: string; targetName: string; legacyExecutor?: boolean; + eslintConfigDir?: string; } const defaultCreateEslintLintProjectOptions: CreateEslintLintProjectOptions = { @@ -107,6 +108,7 @@ function createTestProject( opts: Partial = defaultCreateEslintLintProjectOptions ) { let projectOpts = { ...defaultCreateEslintLintProjectOptions, ...opts }; + projectOpts.eslintConfigDir ??= projectOpts.appRoot; const project: ProjectConfiguration = { name: projectOpts.appName, root: projectOpts.appRoot, @@ -256,6 +258,15 @@ describe('Eslint - Convert Executors To Plugin', () => { dependencies: {}, externalNodes: {}, }; + + tree.write( + 'package.json', + JSON.stringify({ name: 'workspace', version: '0.0.1' }) + ); + fs.createFileSync( + 'package.json', + JSON.stringify({ name: 'workspace', version: '0.0.1' }) + ); }); afterEach(() => { @@ -263,6 +274,30 @@ describe('Eslint - Convert Executors To Plugin', () => { }); describe('--project', () => { + it('should not migrate a target with an invalid eslint config filename', async () => { + const project = createTestProject(tree); + project.targets.lint.options.eslintConfig = '.invalid-eslint-config.json'; + updateProjectConfiguration(tree, project.name, project); + + await expect( + convertToInferred(tree, { project: project.name, skipFormat: true }) + ).rejects.toThrowErrorMatchingInlineSnapshot( + `"The lint target on project "myapp" cannot be migrated. The "eslintConfig" option value (.invalid-eslint-config.json) is not a default config file known by ESLint."` + ); + }); + + it('should not migrate a target with a eslint config not located in the project root or a parent directory', async () => { + const project = createTestProject(tree); + project.targets.lint.options.eslintConfig = `${project.root}/nested/.eslintrc.json`; + updateProjectConfiguration(tree, project.name, project); + + await expect( + convertToInferred(tree, { project: project.name, skipFormat: true }) + ).rejects.toThrowErrorMatchingInlineSnapshot( + `"The lint target on project "myapp" cannot be migrated. The "eslintConfig" option value (myapp/nested/.eslintrc.json) must point to a file in the project root or a parent directory."` + ); + }); + it('should setup a new Eslint plugin and only migrate one specific project', async () => { // ARRANGE const existingProject = createTestProject(tree, { @@ -319,6 +354,28 @@ describe('Eslint - Convert Executors To Plugin', () => { ).toEqual(['myapp/**/*']); }); + it('should setup a new Eslint plugin and only migrate the root project', async () => { + const project = createTestProject(tree, { + appRoot: '.', + appName: 'app1', + }); + createTestProject(tree, { appRoot: 'app2', appName: 'app2' }); + + await convertToInferred(tree, { project: 'app1', skipFormat: true }); + + // project.json modifications + const updatedProject = readProjectConfiguration(tree, project.name); + expect(updatedProject.targets?.lint).toBeUndefined(); + // nx.json modifications + const nxJsonPlugins = readNxJson(tree).plugins; + const eslintPluginRegistrations = nxJsonPlugins.filter( + (plugin): plugin is ExpandedPluginConfiguration => + typeof plugin !== 'string' && plugin.plugin === '@nx/eslint/plugin' + ); + expect(eslintPluginRegistrations.length).toBe(1); + expect(eslintPluginRegistrations[0].include).toStrictEqual(['*']); + }); + it('should add project to existing plugins includes', async () => { // ARRANGE const existingProject = createTestProject(tree, { @@ -443,6 +500,47 @@ describe('Eslint - Convert Executors To Plugin', () => { ).not.toBeDefined(); }); + it('should remove include when all projects are included and there is a root project', async () => { + createTestProject(tree, { appRoot: '.', appName: 'app1' }); + createTestProject(tree, { appRoot: 'app2', appName: 'app2' }); + const nxJson = readNxJson(tree); + nxJson.plugins ??= []; + nxJson.plugins.push({ + plugin: '@nx/eslint/plugin', + include: ['app2/**/*'], + options: { + targetName: 'lint', + }, + }); + updateNxJson(tree, nxJson); + + await convertToInferred(tree, { project: 'app1', skipFormat: true }); + + // nx.json modifications + const nxJsonPlugins = readNxJson(tree).plugins; + const eslintPluginRegistrations = nxJsonPlugins.filter( + (plugin): plugin is ExpandedPluginConfiguration => + typeof plugin !== 'string' && plugin.plugin === '@nx/eslint/plugin' + ); + expect(eslintPluginRegistrations.length).toBe(1); + expect(eslintPluginRegistrations[0].include).toBeUndefined(); + }); + + it('should remove include when it is a single root project', async () => { + createTestProject(tree, { appRoot: '.', appName: 'app1' }); + + await convertToInferred(tree, { project: 'app1', skipFormat: true }); + + // nx.json modifications + const nxJsonPlugins = readNxJson(tree).plugins; + const eslintPluginRegistrations = nxJsonPlugins.filter( + (plugin): plugin is ExpandedPluginConfiguration => + typeof plugin !== 'string' && plugin.plugin === '@nx/eslint/plugin' + ); + expect(eslintPluginRegistrations.length).toBe(1); + expect(eslintPluginRegistrations[0].include).toBeUndefined(); + }); + it('should remove inputs when they are inferred', async () => { const project = createTestProject(tree); project.targets.lint.options.cacheLocation = 'cache-dir'; @@ -559,6 +657,139 @@ describe('Eslint - Convert Executors To Plugin', () => { { externalDependencies: ['eslint', 'eslint-plugin-react'] }, ]); }); + + it.each` + lintFilePatterns | expectedArgs + ${['app1/src']} | ${['src']} + ${['./app1/src']} | ${['src']} + ${['app1/lib']} | ${['lib']} + ${['./app1/lib']} | ${['lib']} + ${['app1/**/*.ts']} | ${['**/*.ts']} + ${['./app1/**/*.ts']} | ${['**/*.ts']} + `( + 'should convert non-inferred lintFilePatterns ($lintFilePatterns) to $expectedArgs in "args" for a nested project', + async ({ lintFilePatterns, expectedArgs }) => { + const project = createTestProject(tree, { + appName: 'app1', + appRoot: 'app1', + }); + project.targets.lint.options.lintFilePatterns = lintFilePatterns; + updateProjectConfiguration(tree, project.name, project); + createTestProject(tree, { + appRoot: 'second', + appName: 'second', + }); + + await convertToInferred(tree, { + project: project.name, + skipFormat: true, + }); + + // project.json modifications + const updatedProject = readProjectConfiguration(tree, project.name); + expect(updatedProject.targets.lint.options.args).toStrictEqual( + expectedArgs + ); + } + ); + + it('should convert non-inferred lintFilePatterns to expectedArgs in "args" for a nested project', async () => { + const project = createTestProject(tree, { + appName: 'app1', + appRoot: 'app1', + }); + project.targets.lint.options.lintFilePatterns = ['./app1/src']; + updateProjectConfiguration(tree, project.name, project); + createTestProject(tree, { + appRoot: 'second', + appName: 'second', + }); + + await convertToInferred(tree, { + project: project.name, + skipFormat: true, + }); + + // project.json modifications + const updatedProject = readProjectConfiguration(tree, project.name); + expect(updatedProject.targets.lint.options.args).toStrictEqual(['src']); + }); + + it('should convert non-inferred lintFilePatterns to project relative patterns in "args" for a root project', async () => { + const project = createTestProject(tree); + project.targets.lint.options.lintFilePatterns = [ + `${project.root}/**/*.ts`, + ]; + updateProjectConfiguration(tree, project.name, project); + createTestProject(tree, { + appRoot: 'second', + appName: 'second', + }); + + await convertToInferred(tree, { + project: project.name, + skipFormat: true, + }); + + // project.json modifications + const updatedProject = readProjectConfiguration(tree, project.name); + expect(updatedProject.targets.lint.options.args).toStrictEqual([ + '**/*.ts', + ]); + }); + + it('should remove "." lintFilePatterns for a nested project', async () => { + const project = createTestProject(tree); + project.targets.lint.options.lintFilePatterns = [project.root]; + updateProjectConfiguration(tree, project.name, project); + createTestProject(tree, { + appRoot: 'second', + appName: 'second', + }); + + await convertToInferred(tree, { + project: project.name, + skipFormat: true, + }); + + // project.json modifications + const updatedProject = readProjectConfiguration(tree, project.name); + expect(updatedProject.targets?.lint).toBeUndefined(); + }); + + it.each` + lintFilePatterns + ${['.']} + ${['./src']} + ${['src']} + ${['./lib']} + ${['lib']} + ${['./src', './lib']} + ${['src', 'lib']} + `( + 'should remove "$lintFilePatterns" lintFilePatterns for a root project', + async ({ lintFilePatterns }) => { + const project = createTestProject(tree, { + appRoot: '.', + appName: 'app1', + }); + project.targets.lint.options.lintFilePatterns = lintFilePatterns; + updateProjectConfiguration(tree, project.name, project); + createTestProject(tree, { + appRoot: 'second', + appName: 'second', + }); + + await convertToInferred(tree, { + project: project.name, + skipFormat: true, + }); + + // project.json modifications + const updatedProject = readProjectConfiguration(tree, project.name); + expect(updatedProject.targets?.lint).toBeUndefined(); + } + ); }); describe('--all', () => { @@ -720,7 +951,6 @@ describe('Eslint - Convert Executors To Plugin', () => { { "options": { "cache-location": "cache-dir", - "config": ".eslintrc.json", }, } `); @@ -761,7 +991,6 @@ describe('Eslint - Convert Executors To Plugin', () => { expect(updatedProject.targets.lint).toMatchInlineSnapshot(` { "options": { - "config": ".eslintrc.json", "max-warnings": 10, }, } diff --git a/packages/eslint/src/generators/convert-to-inferred/convert-to-inferred.ts b/packages/eslint/src/generators/convert-to-inferred/convert-to-inferred.ts index 6114869c37c80..60da20c7c43d6 100644 --- a/packages/eslint/src/generators/convert-to-inferred/convert-to-inferred.ts +++ b/packages/eslint/src/generators/convert-to-inferred/convert-to-inferred.ts @@ -1,18 +1,17 @@ import { createProjectGraphAsync, formatFiles, - names, + type ProjectConfiguration, type TargetConfiguration, type Tree, } from '@nx/devkit'; -import { createNodesV2, EslintPluginOptions } from '../../plugins/plugin'; import { migrateProjectExecutorsToPlugin } from '@nx/devkit/src/generators/plugin-migrations/executor-to-plugin-migrator'; -import { targetOptionsToCliMap } from './lib/target-options-map'; +import { processTargetOutputs } from '@nx/devkit/src/generators/plugin-migrations/plugin-migration-utils'; +import { basename, dirname, relative } from 'node:path/posix'; import { interpolate } from 'nx/src/tasks-runner/utils'; -import { - processTargetOutputs, - toProjectRelativePath, -} from '@nx/devkit/src/generators/plugin-migrations/plugin-migration-utils'; +import { createNodesV2, type EslintPluginOptions } from '../../plugins/plugin'; +import { ESLINT_CONFIG_FILENAMES } from '../../utils/config-file'; +import { targetOptionsToCliMap } from './lib/target-options-map'; interface Schema { project?: string; @@ -34,6 +33,7 @@ export async function convertToInferred(tree: Tree, options: Schema) { executors: ['@nx/eslint:lint', '@nrwl/linter:eslint'], postTargetTransformer, targetPluginOptionMapper: (targetName) => ({ targetName }), + skipTargetFilter, }, ], options.project @@ -107,13 +107,9 @@ function handlePropertiesInOptions( projectDetails: { projectName: string; root: string }, target: TargetConfiguration ) { - if ('eslintConfig' in options) { - options.config = toProjectRelativePath( - options.eslintConfig, - projectDetails.root - ); - delete options.eslintConfig; - } + // inferred targets are only identified after known files that ESLint would + // pick up, so we can remove the eslintConfig option + delete options.eslintConfig; if ('force' in options) { delete options.force; @@ -150,22 +146,61 @@ function handlePropertiesInOptions( if ('lintFilePatterns' in options) { const normalizedLintFilePatterns = options.lintFilePatterns.map( (pattern) => { - return interpolate(pattern, { + const interpolatedPattern = interpolate(pattern, { workspaceRoot: '', projectRoot: projectDetails.root, projectName: projectDetails.projectName, }); + + if (interpolatedPattern === projectDetails.root) { + return '.'; + } + + return interpolatedPattern.replace( + new RegExp(`^(?:\./)?${projectDetails.root}/`), + '' + ); } ); - options.args = normalizedLintFilePatterns.map((pattern) => - pattern.startsWith(projectDetails.root) - ? pattern.replace(new RegExp(`^${projectDetails.root}/`), './') - : pattern - ); + options.args = normalizedLintFilePatterns + // the @nx/eslint/plugin automatically infers these, so we don't need to pass them in + .filter((p) => + projectDetails.root === '.' + ? !['.', 'src', './src', 'lib', './lib'].includes(p) + : p !== '.' + ); + if (options.args.length === 0) { + delete options.args; + } delete options.lintFilePatterns; } } export default convertToInferred; + +function skipTargetFilter( + targetOptions: { eslintConfig?: string }, + project: ProjectConfiguration +) { + if (targetOptions.eslintConfig) { + // check that the eslintConfig option is a default config file known by ESLint + if ( + !ESLINT_CONFIG_FILENAMES.includes(basename(targetOptions.eslintConfig)) + ) { + return `The "eslintConfig" option value (${targetOptions.eslintConfig}) is not a default config file known by ESLint.`; + } + + // check that it is at the project root or in a parent directory + const eslintConfigPath = relative(project.root, targetOptions.eslintConfig); + if ( + dirname(eslintConfigPath) !== '.' && + !eslintConfigPath.startsWith('../') + ) { + return `The "eslintConfig" option value (${targetOptions.eslintConfig}) must point to a file in the project root or a parent directory.`; + } + } + + return false; +}