Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(core): throw an error when a circular dep occurs in a graph mixin… #9714

Merged
merged 1 commit into from Apr 7, 2022
Merged

fix(core): throw an error when a circular dep occurs in a graph mixin… #9714

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
36 changes: 0 additions & 36 deletions packages/nx/src/tasks-runner/__snapshots__/task-graph-creator.spec.ts.snap
View file
Open in desktop
Expand Up @@ -36,42 +36,6 @@ Object {
}
`;

exports[`TaskGraphCreator (tasks with dependency configurations) should create a task graph (builds depend on builds of dependencies with intermediate projects and circular dependencies between projects) 1`] = `
Object {
"dependencies": Object {
"app1:build": Array [
"common2:build",
],
"common2:build": Array [],
},
"roots": Array [
"common2:build",
],
"tasks": Object {
"app1:build": Object {
"id": "app1:build",
"overrides": Object {},
"projectRoot": "app1-root",
"target": Object {
"configuration": undefined,
"project": "app1",
"target": "build",
},
},
"common2:build": Object {
"id": "common2:build",
"overrides": Object {},
"projectRoot": "common2-root",
"target": Object {
"configuration": undefined,
"project": "common2",
"target": "build",
},
},
},
}
`;

exports[`TaskGraphCreator (tasks with dependency configurations) should create a task graph (builds depend on builds of dependencies with intermediate projects and circular dependencies between projects) 2 1`] = `
Object {
"dependencies": Object {
Expand Down
103 changes: 57 additions & 46 deletions packages/nx/src/tasks-runner/run-command.spec.ts
View file
Open in desktop
Expand Up @@ -458,7 +458,8 @@ describe('createTasksForProjectToRun', () => {
});
});

it('should handle circular dependencies between projects', () => {
it('should handle APP1 <=> LIB1(no build) => LIB2', () => {
// APP1 <=> LIB1 => LIB2
// App 1 depends on builds of its dependencies
projectGraph.nodes.app1.data.targets.build.dependsOn = [
{
Expand Down Expand Up @@ -501,13 +502,7 @@ describe('createTasksForProjectToRun', () => {
},
},
};
projectGraph.dependencies.lib2 = [
{
type: DependencyType.static,
source: 'lib2',
target: 'lib1',
},
];
projectGraph.dependencies.lib2 = [];

const tasks = createTasksForProjectToRun(
[projectGraph.nodes.app1],
Expand All @@ -534,24 +529,49 @@ describe('createTasksForProjectToRun', () => {
});
});

it('should handle circular dependencies between projects with no tasks', () => {
// App 1 depends on builds of its dependencies
it('should error when APP1 <=> LIB1(no build) <=> LIB2', () => {
jest.spyOn(process, 'exit').mockImplementation(() => {
throw new Error();
});

// Define Lib 2 with a build
projectGraph.nodes.lib2 = {
name: 'lib2',
type: 'lib',
data: {
root: 'lib2-root',
files: [],
targets: {
build: {},
},
},
};

// Lib 1 does not have "build"
delete projectGraph.nodes.lib1.data.targets.build;

// APP1 <=> LIB1 <=> LIB2
projectGraph.nodes.app1.data.targets.build.dependsOn = [
{
target: 'build',
projects: 'dependencies',
},
];

projectGraph.nodes.lib2.data.targets.build.dependsOn = [
{
target: 'build',
projects: 'dependencies',
},
];

// App 1 depends on Lib 1
projectGraph.dependencies.app1.push({
type: DependencyType.static,
source: 'app1',
target: 'lib1',
});

// Lib 1 does not have build but depends on Lib 2
delete projectGraph.nodes.lib1.data.targets.build;
projectGraph.dependencies.lib1.push(
{
type: DependencyType.static,
Expand All @@ -565,45 +585,36 @@ describe('createTasksForProjectToRun', () => {
}
);

// Lib 2 has a build
projectGraph.nodes.lib2 = {
name: 'lib2',
type: 'lib',
data: {
root: 'lib2-root',
files: [],
targets: {
build: {},
},
},
};
projectGraph.dependencies.lib2 = [];

const tasks = createTasksForProjectToRun(
[projectGraph.nodes.app1],
projectGraph.dependencies.lib2 = [
{
target: 'build',
configuration: undefined,
overrides: {},
type: DependencyType.static,
source: 'lib2',
target: 'lib1',
},
projectGraph,
projectGraph.nodes.app1.name
);
];

expect(tasks).toContainEqual({
id: 'app1:build',
target: { project: 'app1', target: 'build' },
projectRoot: 'app1-root',
overrides: {},
});
expect(tasks).toContainEqual({
id: 'lib2:build',
target: { project: 'lib2', target: 'build' },
projectRoot: 'lib2-root',
overrides: {},
});
try {
createTasksForProjectToRun(
[projectGraph.nodes.app1],
{
target: 'build',
configuration: undefined,
overrides: {},
},
projectGraph,
projectGraph.nodes.app1.name
);
fail();
} catch (e) {
expect(process.exit).toHaveBeenCalledWith(1);
}
});

// Technically, this could work but it creates a lot of problems in the implementation,
// so instead we error saying that the circular dependency cannot be handled.
// xit('should handle APP1 => LIB1(no build) <=> LIB2', () => {
// });

it('should throw an error for an invalid target', () => {
jest.spyOn(process, 'exit').mockImplementation(() => {
throw new Error();
Expand Down
69 changes: 53 additions & 16 deletions packages/nx/src/tasks-runner/run-command.ts
View file
Open in desktop
Expand Up @@ -258,7 +258,7 @@ function addTasksForProjectTarget(
projectGraph: ProjectGraph,
originalTargetExecutor: string,
tasksMap: Map<string, Task>,
path: string[],
path: { targetIdentifier: string; hasTarget: boolean }[],
seenSet: Set<string>
) {
const task = createTask({
Expand Down Expand Up @@ -356,23 +356,22 @@ function addTasksForProjectDependencyConfig(
projectGraph: ProjectGraph,
originalTargetExecutor: string,
tasksMap: Map<string, Task>,
path: string[],
path: { targetIdentifier: string; hasTarget: boolean }[],
seenSet: Set<string>
) {
const targetIdentifier = getId({
project: project.name,
target,
configuration,
});
seenSet.add(project.name);

if (path.includes(targetIdentifier)) {
output.error({
title: `Could not execute ${path[0]} because it has a circular dependency`,
bodyLines: [`${[...path, targetIdentifier].join(' --> ')}`],
});
process.exit(1);
}
const pathFragment = {
targetIdentifier,
hasTarget: projectHasTarget(project, target),
};

const newPath = [...path, pathFragment];
seenSet.add(project.name);

if (tasksMap.has(targetIdentifier)) {
return;
Expand All @@ -385,10 +384,21 @@ function addTasksForProjectDependencyConfig(
const depProject = projectGraph.nodes[
dep.target
] as ProjectGraphProjectNode;

if (
depProject &&
projectHasTarget(depProject, dependencyConfig.target)
) {
const depTargetId = getId({
project: depProject.name,
target,
configuration: configuration,
});
exitOnCircularDep(newPath, depTargetId);
if (seenSet.has(dep.target)) {
continue;
}

addTasksForProjectTarget(
{
project: depProject,
Expand All @@ -401,17 +411,26 @@ function addTasksForProjectDependencyConfig(
projectGraph,
originalTargetExecutor,
tasksMap,
[...path, targetIdentifier],
newPath,
seenSet
);
} else {
if (seenSet.has(dep.target)) {
continue;
}
if (!depProject) {
seenSet.add(dep.target);
continue;
}
const depTargetId = getId({
project: depProject.name,
target,
configuration: configuration,
});

exitOnCircularDep(newPath, depTargetId);

if (seenSet.has(dep.target)) {
continue;
}

addTasksForProjectDependencyConfig(
depProject,
{ target, configuration, overrides },
Expand All @@ -420,7 +439,7 @@ function addTasksForProjectDependencyConfig(
projectGraph,
originalTargetExecutor,
tasksMap,
path,
newPath,
seenSet
);
}
Expand All @@ -439,12 +458,30 @@ function addTasksForProjectDependencyConfig(
projectGraph,
originalTargetExecutor,
tasksMap,
[...path, targetIdentifier],
newPath,
seenSet
);
}
}

function exitOnCircularDep(
path: { targetIdentifier: string; hasTarget: boolean }[],
targetIdentifier: string
) {
if (
path.length > 0 &&
path[path.length - 1].hasTarget &&
path.filter((p) => p.targetIdentifier === targetIdentifier).length > 0
) {
const identifiers = path.map((p) => p.targetIdentifier);
output.error({
title: `Could not execute ${identifiers[0]} because it has a circular dependency`,
bodyLines: [`${[...identifiers, targetIdentifier].join(' --> ')}`],
});
process.exit(1);
}
}

function getId({
project,
target,
Expand Down
32 changes: 0 additions & 32 deletions packages/nx/src/tasks-runner/task-graph-creator.spec.ts
View file
Open in desktop
Expand Up @@ -320,38 +320,6 @@ describe('TaskGraphCreator', () => {
expect(taskGraph).toMatchSnapshot();
});

it('should create a task graph (builds depend on builds of dependencies with intermediate projects and circular dependencies between projects)', () => {
delete projectGraph.nodes.common1.data.targets.build;
projectGraph.dependencies.common1.push({
type: DependencyType.static,
source: 'common1',
target: 'common2',
});

projectGraph.dependencies.common2.push({
type: DependencyType.static,
source: 'common2',
target: 'common1',
});

const tasks = createTasksForProjectToRun(
[projectGraph.nodes.app1],
{
target: 'build',
configuration: undefined,
overrides: {},
},
projectGraph,
null
);

const taskGraph = new TaskGraphCreator(projectGraph, {}).createTaskGraph(
tasks
);

expect(taskGraph).toMatchSnapshot();
});

it('should create a task graph (builds depend on builds of dependencies with intermediate projects and circular dependencies between projects) 2', () => {
delete projectGraph.nodes.common1.data.targets.build;
projectGraph.dependencies.common1.push({
Expand Down
6 changes: 5 additions & 1 deletion packages/nx/src/utils/project-graph-utils.ts
View file
Open in desktop
Expand Up @@ -10,7 +10,11 @@ export function projectHasTarget(
project: ProjectGraphProjectNode,
target: string
) {
return project.data && project.data.targets && project.data.targets[target];
return !!(
project.data &&
project.data.targets &&
project.data.targets[target]
);
}

export function projectHasTargetAndConfiguration(
Expand Down