From ad381de2bd921934b343a3b281fec187a290d392 Mon Sep 17 00:00:00 2001 From: AgentEnder Date: Wed, 16 Aug 2023 23:23:25 -0400 Subject: [PATCH] fix(core): name collisions during project inference should not error out if corrected by a project.json's name --- docs/generated/devkit/CreateNodesContext.md | 7 - .../generators/utils/project-configuration.ts | 24 ++- .../utils/project-configuration-utils.spec.ts | 150 +++++++++++++++++- .../utils/project-configuration-utils.ts | 102 +++++++----- packages/nx/src/utils/nx-plugin.ts | 4 +- 5 files changed, 227 insertions(+), 60 deletions(-) diff --git a/docs/generated/devkit/CreateNodesContext.md b/docs/generated/devkit/CreateNodesContext.md index 6c3f5a38ddc99..801549837e1e8 100644 --- a/docs/generated/devkit/CreateNodesContext.md +++ b/docs/generated/devkit/CreateNodesContext.md @@ -7,7 +7,6 @@ Context for [CreateNodesFunction](../../devkit/documents/CreateNodesFunction) ### Properties - [nxJsonConfiguration](../../devkit/documents/CreateNodesContext#nxjsonconfiguration) -- [projectsConfigurations](../../devkit/documents/CreateNodesContext#projectsconfigurations) - [workspaceRoot](../../devkit/documents/CreateNodesContext#workspaceroot) ## Properties @@ -18,12 +17,6 @@ Context for [CreateNodesFunction](../../devkit/documents/CreateNodesFunction) --- -### projectsConfigurations - -• `Readonly` **projectsConfigurations**: `Record`<`string`, [`ProjectConfiguration`](../../devkit/documents/ProjectConfiguration)\> - ---- - ### workspaceRoot • `Readonly` **workspaceRoot**: `string` diff --git a/packages/nx/src/generators/utils/project-configuration.ts b/packages/nx/src/generators/utils/project-configuration.ts index 5930ddbb95a30..fae4f565335f9 100644 --- a/packages/nx/src/generators/utils/project-configuration.ts +++ b/packages/nx/src/generators/utils/project-configuration.ts @@ -10,7 +10,10 @@ import { ProjectConfiguration, ProjectsConfigurations, } from '../../config/workspace-json-project-json'; -import { mergeProjectConfigurationIntoProjectsConfigurations } from '../../project-graph/utils/project-configuration-utils'; +import { + mergeProjectConfigurationIntoRootMap, + readProjectConfigurationsFromRootMap, +} from '../../project-graph/utils/project-configuration-utils'; import { retrieveProjectConfigurationPathsWithoutPluginInference } from '../../project-graph/utils/retrieve-workspace-files'; import { output } from '../../utils/output'; import { PackageJson } from '../../utils/package-json'; @@ -207,17 +210,12 @@ function readAndCombineAllProjectConfigurations(tree: Tree): { (r) => deletedFiles.indexOf(r) === -1 ); - const rootMap: Map = new Map(); - return projectFiles.reduce((projects, projectFile) => { + const rootMap: Map = new Map(); + for (const projectFile of projectFiles) { if (basename(projectFile) === 'project.json') { const json = readJson(tree, projectFile); const config = buildProjectFromProjectJson(json, projectFile); - mergeProjectConfigurationIntoProjectsConfigurations( - projects, - rootMap, - config, - projectFile - ); + mergeProjectConfigurationIntoRootMap(rootMap, config, projectFile); } else { const packageJson = readJson(tree, projectFile); const config = buildProjectConfigurationFromPackageJson( @@ -225,8 +223,7 @@ function readAndCombineAllProjectConfigurations(tree: Tree): { projectFile, readNxJson(tree) ); - mergeProjectConfigurationIntoProjectsConfigurations( - projects, + mergeProjectConfigurationIntoRootMap( rootMap, // Inferred targets, tags, etc don't show up when running generators // This is to help avoid running into issues when trying to update the workspace @@ -237,8 +234,9 @@ function readAndCombineAllProjectConfigurations(tree: Tree): { projectFile ); } - return projects; - }, {}); + } + + return readProjectConfigurationsFromRootMap(rootMap); } /** diff --git a/packages/nx/src/project-graph/utils/project-configuration-utils.spec.ts b/packages/nx/src/project-graph/utils/project-configuration-utils.spec.ts index b3560434c77d4..6893da7120eee 100644 --- a/packages/nx/src/project-graph/utils/project-configuration-utils.spec.ts +++ b/packages/nx/src/project-graph/utils/project-configuration-utils.spec.ts @@ -1,6 +1,11 @@ -import { TargetConfiguration } from '../../config/workspace-json-project-json'; import { + ProjectConfiguration, + TargetConfiguration, +} from '../../config/workspace-json-project-json'; +import { + mergeProjectConfigurationIntoRootMap, mergeTargetConfigurations, + readProjectConfigurationsFromRootMap, readTargetDefaultsForTarget, } from './project-configuration-utils'; @@ -352,3 +357,146 @@ describe('target defaults', () => { }); }); }); + +describe('mergeProjectConfigurationIntoRootMap', () => { + it('should merge targets from different configurations', () => { + const rootMap = new RootMapBuilder() + .addProject({ + root: 'libs/lib-a', + name: 'lib-a', + targets: { + echo: { + command: 'echo lib-a', + }, + }, + }) + .getRootMap(); + mergeProjectConfigurationIntoRootMap( + rootMap, + { + root: 'libs/lib-a', + name: 'lib-a', + targets: { + build: { + command: 'tsc', + }, + }, + }, + 'inferred-project-config-file.ts' + ); + expect(rootMap.get('libs/lib-a')).toMatchInlineSnapshot(` + { + "name": "lib-a", + "root": "libs/lib-a", + "targets": { + "build": { + "command": "tsc", + }, + "echo": { + "command": "echo lib-a", + }, + }, + } + `); + }); + + it("shouldn't overwrite project name, unless merging project from project.json", () => { + const rootMap = new RootMapBuilder() + .addProject({ + name: 'bad-name', + root: 'libs/lib-a', + }) + .getRootMap(); + mergeProjectConfigurationIntoRootMap( + rootMap, + { + name: 'other-bad-name', + root: 'libs/lib-a', + }, + 'libs/lib-a/package.json' + ); + expect(rootMap.get('libs/lib-a').name).toEqual('bad-name'); + mergeProjectConfigurationIntoRootMap( + rootMap, + { + name: 'lib-a', + root: 'libs/lib-a', + }, + 'libs/lib-a/project.json' + ); + expect(rootMap.get('libs/lib-a').name).toEqual('lib-a'); + }); +}); + +describe('readProjectsConfigurationsFromRootMap', () => { + it('should error if multiple roots point to the same project', () => { + const rootMap = new RootMapBuilder() + .addProject({ + name: 'lib', + root: 'apps/lib-a', + }) + .addProject({ + name: 'lib', + root: 'apps/lib-b', + }) + .getRootMap(); + + expect(() => { + readProjectConfigurationsFromRootMap(rootMap); + }).toThrowErrorMatchingInlineSnapshot(` + "The following projects are defined in multiple locations: + - lib: + - apps/lib-a + - apps/lib-b + + To fix this, set a unique name for each project in a project.json inside the project's root. If the project does not currently have a project.json, you can create one that contains only a name." + `); + }); + + it('should read root map into standard projects configurations form', () => { + const rootMap = new RootMapBuilder() + .addProject({ + name: 'lib-a', + root: 'libs/a', + }) + .addProject({ + name: 'lib-b', + root: 'libs/b', + }) + .addProject({ + name: 'lib-shared-b', + root: 'libs/shared/b', + }) + .getRootMap(); + expect(readProjectConfigurationsFromRootMap(rootMap)) + .toMatchInlineSnapshot(` + { + "lib-a": { + "name": "lib-a", + "root": "libs/a", + }, + "lib-b": { + "name": "lib-b", + "root": "libs/b", + }, + "lib-shared-b": { + "name": "lib-shared-b", + "root": "libs/shared/b", + }, + } + `); + }); +}); + +class RootMapBuilder { + private rootMap: Map = new Map(); + + addProject(p: ProjectConfiguration) { + this.rootMap.set(p.root, p); + return this; + } + + getRootMap() { + return this.rootMap; + } +} diff --git a/packages/nx/src/project-graph/utils/project-configuration-utils.ts b/packages/nx/src/project-graph/utils/project-configuration-utils.ts index d00be15f65a04..68e01ff8faea9 100644 --- a/packages/nx/src/project-graph/utils/project-configuration-utils.ts +++ b/packages/nx/src/project-graph/utils/project-configuration-utils.ts @@ -8,53 +8,42 @@ import { ProjectConfiguration, TargetConfiguration, } from '../../config/workspace-json-project-json'; -import { readJsonFile } from '../../utils/fileutils'; import { NX_PREFIX } from '../../utils/logger'; import { NxPluginV2 } from '../../utils/nx-plugin'; import { workspaceRoot } from '../../utils/workspace-root'; import minimatch = require('minimatch'); -export function mergeProjectConfigurationIntoProjectsConfigurations( - // projectName -> ProjectConfiguration - existingProjects: Record, - // projectRoot -> projectName - existingProjectRootMap: Map, + +export function mergeProjectConfigurationIntoRootMap( + projectRootMap: Map, project: ProjectConfiguration, // project.json is a special case, so we need to detect it. file: string ): void { - let matchingProjectName = existingProjectRootMap.get(project.root); + const matchingProject = projectRootMap.get(project.root); - if (!matchingProjectName) { - existingProjects[project.name] = project; - existingProjectRootMap.set(project.root, project.name); + if (!matchingProject) { + projectRootMap.set(project.root, project); return; - // There are some special cases for handling project.json - mainly - // that it should override any name the project already has. } else if ( project.name && - project.name !== matchingProjectName && + project.name !== matchingProject.name && basename(file) === 'project.json' ) { - // Copy config to new name - existingProjects[project.name] = existingProjects[matchingProjectName]; - // Update name in project config - existingProjects[project.name].name = project.name; - // Update root map to point to new name - existingProjectRootMap[project.root] = project.name; - // Remove entry for old name - delete existingProjects[matchingProjectName]; - // Update name that config should be merged to - matchingProjectName = project.name; + // `name` inside project.json overrides any names from + // inference plugins + matchingProject.name = project.name; } - const matchingProject = existingProjects[matchingProjectName]; - - // This handles top level properties that are overwritten. `srcRoot`, `projectType`, or fields that Nx doesn't know about. + // This handles top level properties that are overwritten. + // e.g. `srcRoot`, `projectType`, or other fields that shouldn't be extended + // Note: `name` is set specifically here to keep it from changing. The name is + // always determined by the first inference plugin to ID a project, unless it has + // a project.json in which case it was already updated above. const updatedProjectConfiguration = { ...matchingProject, ...project, - name: matchingProjectName, + name: matchingProject.name, }; // The next blocks handle properties that should be themselves merged (e.g. targets, tags, and implicit dependencies) @@ -83,11 +72,10 @@ export function mergeProjectConfigurationIntoProjectsConfigurations( }; } - if (updatedProjectConfiguration.name !== matchingProject.name) { - delete existingProjects[matchingProject.name]; - } - existingProjects[updatedProjectConfiguration.name] = - updatedProjectConfiguration; + projectRootMap.set( + updatedProjectConfiguration.root, + updatedProjectConfiguration + ); } export function buildProjectsConfigurationsFromProjectPathsAndPlugins( @@ -99,8 +87,7 @@ export function buildProjectsConfigurationsFromProjectPathsAndPlugins( projects: Record; externalNodes: Record; } { - const projectRootMap: Map = new Map(); - const projects: Record = {}; + const projectRootMap: Map = new Map(); const externalNodes: Record = {}; // We push the nx core node builder onto the end, s.t. it overwrites any user specified behavior @@ -119,13 +106,11 @@ export function buildProjectsConfigurationsFromProjectPathsAndPlugins( if (minimatch(file, pattern)) { const { projects: projectNodes, externalNodes: pluginExternalNodes } = configurationConstructor(file, { - projectsConfigurations: projects, nxJsonConfiguration: nxJson, workspaceRoot: root, }); for (const node in projectNodes) { - mergeProjectConfigurationIntoProjectsConfigurations( - projects, + mergeProjectConfigurationIntoRootMap( projectRootMap, projectNodes[node], file @@ -136,7 +121,48 @@ export function buildProjectsConfigurationsFromProjectPathsAndPlugins( } } - return { projects, externalNodes }; + return { + projects: readProjectConfigurationsFromRootMap(projectRootMap), + externalNodes, + }; +} + +export function readProjectConfigurationsFromRootMap( + projectRootMap: Map +) { + const projects: Record = {}; + // If there are projects that have the same name, that is an error. + // This object tracks name -> (all roots of projects with that name) + // to provide better error messaging. + const errors: Map = new Map(); + + for (const [root, configuration] of projectRootMap.entries()) { + if (!configuration.name) { + throw new Error(`Project at ${root} has no name provided.`); + } else if (configuration.name in projects) { + let rootErrors = errors.get(configuration.name) ?? [ + projects[configuration.name].root, + ]; + rootErrors.push(root); + errors.set(configuration.name, rootErrors); + } else { + projects[configuration.name] = configuration; + } + } + + if (errors.size > 0) { + throw new Error( + [ + `The following projects are defined in multiple locations:`, + ...Array.from(errors.entries()).map(([project, roots]) => + [`- ${project}: `, ...roots.map((r) => ` - ${r}`)].join('\n') + ), + '', + "To fix this, set a unique name for each project in a project.json inside the project's root. If the project does not currently have a project.json, you can create one that contains only a name.", + ].join('\n') + ); + } + return projects; } export function mergeTargetConfigurations( diff --git a/packages/nx/src/utils/nx-plugin.ts b/packages/nx/src/utils/nx-plugin.ts index 3632249c14254..fa1e70991ec69 100644 --- a/packages/nx/src/utils/nx-plugin.ts +++ b/packages/nx/src/utils/nx-plugin.ts @@ -42,7 +42,6 @@ import { combineGlobPatterns } from './globs'; * Context for {@link CreateNodesFunction} */ export interface CreateNodesContext { - readonly projectsConfigurations: Record; readonly nxJsonConfiguration: NxJsonConfiguration; readonly workspaceRoot: string; } @@ -262,6 +261,9 @@ export async function loadNxPlugins( } function ensurePluginIsV2(plugin: NxPlugin): NxPluginV2 { + if (isNxPluginV2(plugin)) { + return plugin; + } if (isNxPluginV1(plugin) && plugin.projectFilePatterns) { return { ...plugin,