fix(core): stabilizes project references in dependsOn and inputs when later plugins rename a project#34332
fix(core): stabilizes project references in dependsOn and inputs when later plugins rename a project#34332AgentEnder wants to merge 6 commits intomasterfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
View your CI Pipeline Execution ↗ for commit 1f167a9
☁️ Nx Cloud last updated this comment at |
✅ Deploy Preview for nx-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for nx-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
ba435cc to
615e53a
Compare
… later plugins rename a project
615e53a to
71a0422
Compare
| registerSubstitutorsForNodeResults( | ||
| pluginResultProjects?: Record< | ||
| string, | ||
| Omit<ProjectConfiguration, 'root'> & Partial<ProjectConfiguration> | ||
| >, | ||
| mergedConfigurations?: Record<string, ProjectConfiguration> | ||
| ) { | ||
| const nameMap = new AggregateMap( | ||
| rootMapToNameMap(pluginResultProjects), | ||
| new LazyMap(() => rootMapToNameMap(mergedConfigurations)) | ||
| ); | ||
|
|
||
| for (const root in pluginResultProjects) { | ||
| const project = pluginResultProjects[root]; |
There was a problem hiding this comment.
Missing null/undefined check for pluginResultProjects parameter before iteration. The test on line 88-91 of the spec file shows this method should handle undefined inputs gracefully, but the code will throw a runtime error when trying to iterate over undefined.
registerSubstitutorsForNodeResults(
pluginResultProjects?: Record<...>,
mergedConfigurations?: Record<string, ProjectConfiguration>
) {
// Add this guard
if (!pluginResultProjects) {
return;
}
const nameMap = new AggregateMap(
rootMapToNameMap(pluginResultProjects),
new LazyMap(() => rootMapToNameMap(mergedConfigurations))
);
for (const root in pluginResultProjects) {
// ...
}
}| registerSubstitutorsForNodeResults( | |
| pluginResultProjects?: Record< | |
| string, | |
| Omit<ProjectConfiguration, 'root'> & Partial<ProjectConfiguration> | |
| >, | |
| mergedConfigurations?: Record<string, ProjectConfiguration> | |
| ) { | |
| const nameMap = new AggregateMap( | |
| rootMapToNameMap(pluginResultProjects), | |
| new LazyMap(() => rootMapToNameMap(mergedConfigurations)) | |
| ); | |
| for (const root in pluginResultProjects) { | |
| const project = pluginResultProjects[root]; | |
| registerSubstitutorsForNodeResults( | |
| pluginResultProjects?: Record< | |
| string, | |
| Omit<ProjectConfiguration, 'root'> & Partial<ProjectConfiguration> | |
| >, | |
| mergedConfigurations?: Record<string, ProjectConfiguration> | |
| ) { | |
| if (!pluginResultProjects) { | |
| return; | |
| } | |
| const nameMap = new AggregateMap( | |
| rootMapToNameMap(pluginResultProjects), | |
| new LazyMap(() => rootMapToNameMap(mergedConfigurations)) | |
| ); | |
| for (const root in pluginResultProjects) { | |
| const project = pluginResultProjects[root]; |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
| get(key: K): V | null { | ||
| if (!this.data) { | ||
| this.data = this.builder(); | ||
| } | ||
| return this.data.get(key); | ||
| } |
There was a problem hiding this comment.
Type mismatch between declared return type and actual return value. The method declares return type V | null but this.data.get(key) returns V | undefined. This will cause undefined to be returned when a key is not found, not null as declared.
get(key: K): V | undefined { // Fix return type
if (!this.data) {
this.data = this.builder();
}
return this.data.get(key);
}| get(key: K): V | null { | |
| if (!this.data) { | |
| this.data = this.builder(); | |
| } | |
| return this.data.get(key); | |
| } | |
| get(key: K): V | undefined { | |
| if (!this.data) { | |
| this.data = this.builder(); | |
| } | |
| return this.data.get(key); | |
| } |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
| const project = { | ||
| root: node, | ||
| ...projectNodes[node], | ||
| root: root, |
There was a problem hiding this comment.
| root: root, | |
| root, |
| private projectNameSubstitutors = new Map<string, ProjectNameSubstitutor[]>(); | ||
| private dirtyRoots = new Set<string>(); | ||
|
|
||
| constructor() {} |
There was a problem hiding this comment.
| constructor() {} |
| string, | ||
| Omit<ProjectConfiguration, 'root'> & Partial<ProjectConfiguration> | ||
| >, | ||
| mergedConfigurations?: Record<string, ProjectConfiguration> |
There was a problem hiding this comment.
| mergedConfigurations?: Record<string, ProjectConfiguration> | |
| mergedRootMap?: Record<string, ProjectConfiguration> |
|
|
||
| if (project.targets) { | ||
| for (const target in project.targets) { | ||
| const config = project.targets[target]; |
There was a problem hiding this comment.
| const config = project.targets[target]; | |
| const targetConfig = project.targets[target]; |
| if (config.inputs) { | ||
| for (const input of config.inputs) { | ||
| if (typeof input === 'object' && 'projects' in input) { | ||
| const projects = input['projects']; |
There was a problem hiding this comment.
| const projects = input['projects']; | |
| const inputProjectNames = input.projects; |
| // names. | ||
| for (let i = 0; i < dep.projects.length; i++) { | ||
| const projectName = dep.projects[i]; | ||
| if (!isGlobPattern(projectName)) { |
There was a problem hiding this comment.
| if (!isGlobPattern(projectName)) { |
| if (project.targets) { | ||
| for (const target in project.targets) { | ||
| const config = project.targets[target]; | ||
| if (config.inputs) { |
There was a problem hiding this comment.
extract this into registerSubstitutorsForInputs
| } | ||
| } | ||
|
|
||
| if (config.dependsOn) { |
There was a problem hiding this comment.
extract this into registerSubstitutorsForDependsOn
| } | ||
|
|
||
| markDirty(root: string) { | ||
| // It can't be dirty yet if there's no substitutors pointing at it |
There was a problem hiding this comment.
This feels incorrect? What is the downside of saying it's always dirty?
| if (project) { | ||
| this.registerProjectNameSubstitutor( | ||
| project.root, | ||
| (name) => { |
There was a problem hiding this comment.
| (name) => { | |
| (finalName) => { |
c45117e to
45deaa9
Compare
Co-authored-by: AgentEnder <AgentEnder@users.noreply.github.com>
| } else if (Array.isArray(inputProjectNames)) { | ||
| for (let j = 0; j < inputProjectNames.length; j++) { | ||
| const projectName = inputProjectNames[j]; | ||
| const referencedProject = nameMap.get(projectName); | ||
| if (referencedProject) { | ||
| const arrayIndex = j; // Capture j by value | ||
| this.registerProjectNameSubstitutor( | ||
| referencedProject.root, | ||
| ownerRoot, | ||
| arrayKey, | ||
| i, | ||
| (finalName, ownerConfig) => { | ||
| const finalInput = | ||
| ownerConfig.targets?.[targetName]?.inputs?.[i]; | ||
| if ( | ||
| finalInput && | ||
| typeof finalInput === 'object' && | ||
| 'projects' in finalInput | ||
| ) { | ||
| (finalInput['projects'] as string[])[arrayIndex] = finalName; | ||
| } | ||
| } | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical bug: When inputProjectNames is an array containing multiple project names (e.g., ['project-b', 'project-c']), all substitutors are registered with the same arrayKey and index i. This causes each subsequent call to registerProjectNameSubstitutor to invoke clearSubstitutorAtIndex(arrayKey, i), which removes the previous substitutor for that index.
For example, if inputs[0].projects = ['project-b', 'project-c']:
- First iteration (j=0): registers substitutor for 'project-b' at index 0
- Second iteration (j=1): calls
clearSubstitutorAtIndex(arrayKey, 0), removing the 'project-b' substitutor, then registers 'project-c'
Result: Only the last project name in the array gets a substitutor, so earlier projects won't be updated when renamed.
Fix: The tracking key should include both i and j to uniquely identify each project reference:
const trackingKey = `${arrayKey}.${i}.projects.${j}`;
this.registerProjectNameSubstitutor(
referencedProject.root,
ownerRoot,
trackingKey, // Use unique key per project in array
i,
(finalName, ownerConfig) => { /* ... */ }
);Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
✅ The fix from Nx Cloud was applied
We've fixed a bug in the ProjectNameInNodePropsManager where multiple project names within a single inputs or dependsOn array (e.g., ['project-b', 'project-c']) would overwrite each other's substitutors, causing only the last project name to be tracked. The fix introduces a three-level tracking key (arrayKey, index, subIndex) to ensure each project name in an array maintains its own substitutor, allowing all project references to be correctly updated when plugins rename projects.
Tip
✅ We verified this fix by re-running nx:test.
Warning
The suggested diff is too large to display here, but you can view it on Nx Cloud ↗
View interactive diff ↗
➡️ This fix was applied by Craigory Coppola
🎓 Learn more about Self-Healing CI on nx.dev
Co-authored-by: AgentEnder <AgentEnder@users.noreply.github.com>
Current Behavior
There's a bug currently where is a plugin returns a dependsOn dependency or input that directly references another project by name, and a later plugin renames that project, the dependsOn or input entry is left stale and pointing at a now non-existent project.
Expected Behavior
The old refs are kept up to date as the nodes get merged together
AI Summary
This pull request introduces a new mechanism to handle project name substitutions in the Nx project graph, ensuring that references to project names in
inputsanddependsOnblocks remain accurate even if a plugin changes a project's name during graph construction. The main addition is theProjectNameInNodePropsManager, which tracks and updates references when project names change. Several related refactorings and improvements were made to integrate this manager into the project configuration merging process.Project name substitution and consistency:
ProjectNameInNodePropsManagerclass to manage and apply project name substitutions when project names change, ensuring that all references ininputsanddependsOnblocks remain consistent.ProjectNameInNodePropsManagerinto themergeCreateNodesResultsfunction, registering substitutors for node results, marking roots as dirty when names change, and applying substitutions after merging. [1] [2] [3]API and function changes:
mergeProjectConfigurationIntoRootMapto return an object indicating whether a project name was changed, instead of just returning void. [1] [2]Code organization and import cleanup:
project-configuration-utils.tsfor better organization and to accommodate the new manager. [1] [2]