ProjectGraph: Only resolve optional dependencies if the target can be reached from the root project

Checking for a non-optional dependency is not sufficient since that can
originate from a project which is optional itself (i.e. has no
non-optional dependency leading to it).

Author: RandomByte

lib/graph/ProjectGraph.js

@@ -229,30 +229,30 @@ class ProjectGraph {
 	}
 
 	/**
-	 * Transforms any optional dependencies in the graph for which the target is referenced by
-	 * at least one non-optional project
-	 * into a non-optional dependency.
+	 * Transforms any optional dependencies declared in the graph to non-optional dependency, if the target
+	 * can already be reached from the root project.
 	 *
 	 * @public
 	 */
-	resolveOptionalDependencies() {
-		// TODO: If a project is referenced as non-optional by an *optional* dependency, it should still be optional
+	async resolveOptionalDependencies() {
 		if (!this._shouldResolveOptionalDependencies) {
 			log.verbose(`Skipping resolution of optional dependencies since none have been declared`);
 			return;
 		}
 		log.verbose(`Resolving optional dependencies...`);
+
+		// First collect all projects that are currently reachable from the root project (=all non-optional projects)
 		const resolvedProjects = new Set;
-		for (const [, dependencies] of Object.entries(this._adjList)) {
-			for (let i = dependencies.length - 1; i >= 0; i--) {
-				resolvedProjects.add(dependencies[i]);
-			}
-		}
+		await this.traverseBreadthFirst(({project}) => {
+			resolvedProjects.add(project.getName());
+		});
+
 		for (const [projectName, dependencies] of Object.entries(this._optAdjList)) {
 			for (let i = dependencies.length - 1; i >= 0; i--) {
 				const targetProjectName = dependencies[i];
 				if (resolvedProjects.has(targetProjectName)) {
-					// Resolve optional dependency
+					// Target node is already reachable in the graph
+					// => Resolve optional dependency
 					log.verbose(`Resolving optional dependency from ${projectName} to ${targetProjectName}...`);
 
 					if (this._adjList[targetProjectName].includes(projectName)) {

test/lib/graph/ProjectGraph.js

@@ -463,7 +463,7 @@ test("resolveOptionalDependencies", async (t) => {
 	graph.declareDependency("library.d", "library.b");
 	graph.declareDependency("library.d", "library.c");
 
-	graph.resolveOptionalDependencies();
+	await graph.resolveOptionalDependencies();
 
 	t.is(graph.isOptionalDependency("library.a", "library.b"), false,
 		"library.a should have no optional dependency to library.b anymore");
@@ -478,7 +478,6 @@ test("resolveOptionalDependencies", async (t) => {
 	]);
 });
 
-
 test("resolveOptionalDependencies: Optional dependency has not been resolved", async (t) => {
 	const {ProjectGraph} = t.context;
 	const graph = new ProjectGraph({
@@ -493,7 +492,7 @@ test("resolveOptionalDependencies: Optional dependency has not been resolved", a
 	graph.declareOptionalDependency("library.a", "library.c");
 	graph.declareDependency("library.a", "library.d");
 
-	graph.resolveOptionalDependencies();
+	await graph.resolveOptionalDependencies();
 
 	t.is(graph.isOptionalDependency("library.a", "library.b"), true,
 		"Dependency from library.a to library.b should still be optional");
@@ -507,6 +506,90 @@ test("resolveOptionalDependencies: Optional dependency has not been resolved", a
 	]);
 });
 
+test("resolveOptionalDependencies: Dependency of optional dependency has not been resolved", async (t) => {
+	const {ProjectGraph} = t.context;
+	const graph = new ProjectGraph({
+		rootProjectName: "library.a"
+	});
+	graph.addProject(await createProject("library.a"));
+	graph.addProject(await createProject("library.b"));
+	graph.addProject(await createProject("library.c"));
+	graph.addProject(await createProject("library.d"));
+
+	graph.declareOptionalDependency("library.a", "library.b");
+	graph.declareOptionalDependency("library.a", "library.c");
+	graph.declareDependency("library.b", "library.c");
+
+	await graph.resolveOptionalDependencies();
+
+	t.is(graph.isOptionalDependency("library.a", "library.b"), true,
+		"Dependency from library.a to library.b should still be optional");
+
+	t.is(graph.isOptionalDependency("library.a", "library.c"), true,
+		"Dependency from library.a to library.c should still be optional");
+
+	await traverseDepthFirst(t, graph, [
+		"library.a"
+	]);
+});
+
+test("resolveOptionalDependencies: Cyclic optional dependency is not resolved", async (t) => {
+	const {ProjectGraph} = t.context;
+	const graph = new ProjectGraph({
+		rootProjectName: "library.a"
+	});
+	graph.addProject(await createProject("library.a"));
+	graph.addProject(await createProject("library.b"));
+	graph.addProject(await createProject("library.c"));
+
+	graph.declareDependency("library.a", "library.b");
+	graph.declareDependency("library.a", "library.c");
+	graph.declareDependency("library.c", "library.b");
+	graph.declareOptionalDependency("library.b", "library.c");
+
+	await graph.resolveOptionalDependencies();
+
+	t.is(graph.isOptionalDependency("library.b", "library.c"), true,
+		"Dependency from library.b to library.c should still be optional");
+
+	await traverseDepthFirst(t, graph, [
+		"library.b",
+		"library.c",
+		"library.a"
+	]);
+});
+
+test("resolveOptionalDependencies: Resolves transitive optional dependencies", async (t) => {
+	const {ProjectGraph} = t.context;
+	const graph = new ProjectGraph({
+		rootProjectName: "library.a"
+	});
+	graph.addProject(await createProject("library.a"));
+	graph.addProject(await createProject("library.b"));
+	graph.addProject(await createProject("library.c"));
+	graph.addProject(await createProject("library.d"));
+
+	graph.declareDependency("library.a", "library.b");
+	graph.declareOptionalDependency("library.a", "library.c");
+	graph.declareDependency("library.b", "library.c");
+	graph.declareDependency("library.c", "library.d");
+	graph.declareOptionalDependency("library.a", "library.d");
+
+	await graph.resolveOptionalDependencies();
+
+	t.is(graph.isOptionalDependency("library.a", "library.c"), false,
+		"Dependency from library.a to library.c should not be optional anymore");
+
+	t.is(graph.isOptionalDependency("library.a", "library.d"), false,
+		"Dependency from library.a to library.d should not be optional anymore");
+
+	await traverseDepthFirst(t, graph, [
+		"library.d",
+		"library.c",
+		"library.b",
+		"library.a"
+	]);
+});
 
 test("traverseBreadthFirst", async (t) => {
 	const {ProjectGraph} = t.context;