fix(linter): convert root projects correctly to inferred and remove default option values (#27035)

<!-- Please make sure you have read the submission guidelines before
posting an PR -->
<!--
https://github.com/nrwl/nx/blob/master/CONTRIBUTING.md#-submitting-a-pr
-->

<!-- Please make sure that your commit message follows our format -->
<!-- Example: `fix(nx): must begin with lowercase` -->

<!-- If this is a particularly complex change or feature addition, you
can request a dedicated Nx release for this pull request branch. Mention
someone from the Nx team or the `@nrwl/nx-pipelines-reviewers` and they
will confirm if the PR warrants its own release for testing purposes,
and generate it for you if appropriate. -->

## Current Behavior
<!-- This is the behavior we have today -->

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
<!-- This is the behavior we should expect with the changes in this PR
-->

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)
<!-- Please link the issue being fixed so it gets closed when this is
merged. -->

Fixes #26775
This commit is contained in:
Leosvel Pérez Espinosa 2024-07-22 16:03:19 +02:00 committed by GitHub
parent 0bc889838f
commit 5217c3385f
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 321 additions and 55 deletions

View File

@ -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<TargetConfiguration>;
type SkipTargetFilter = (
targetConfiguration: TargetConfiguration
targetOptions: Record<string, unknown>,
projectConfiguration: ProjectConfiguration
) => false | string;
type SkipProjectFilter = (
projectConfiguration: ProjectConfiguration
@ -58,7 +60,6 @@ class ExecutorToPluginMigrator<T> {
#nxJson: NxJsonConfiguration;
#targetDefaultsForExecutor: Partial<TargetConfiguration>;
#targetAndProjectsToMigrate: Map<string, Set<string>>;
#pluginToAddForTarget: Map<string, ExpandedPluginConfiguration<T>>;
#createNodes?: CreateNodes<T>;
#createNodesV2?: CreateNodesV2<T>;
#createNodesResultsForTargets: Map<string, ConfigurationResult>;
@ -108,7 +109,6 @@ class ExecutorToPluginMigrator<T> {
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<T> {
}
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<T> {
}
updateProjectConfiguration(this.tree, projectName, projectConfig);
return `${projectFromGraph.data.root}/**/*`;
}
#mergeInputs(
@ -237,7 +228,12 @@ class ExecutorToPluginMigrator<T> {
forEachExecutorOptions(
this.tree,
this.#executor,
(targetConfiguration, projectName, targetName, configurationName) => {
(
options: Record<string, unknown>,
projectName,
targetName,
configurationName
) => {
if (this.#skippedProjects.has(projectName) || configurationName) {
return;
}
@ -263,9 +259,12 @@ class ExecutorToPluginMigrator<T> {
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<T>(
)
);
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 = {

View File

@ -1,24 +1,24 @@
import {
addProjectConfiguration as _addProjectConfiguration,
joinPathFragments,
readJson,
readNxJson,
readProjectConfiguration,
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 { 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,
} from '@nx/devkit';
import { TempFs } from '@nx/devkit/internal-testing-utils';
import { join } from 'node:path';
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<CreateEslintLintProjectOptions> = 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,
},
}

View File

@ -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;
}