diff --git a/change/change-2e852706-e348-4268-baee-efe7ed5475ca.json b/change/change-2e852706-e348-4268-baee-efe7ed5475ca.json new file mode 100644 index 000000000..d7503fa01 --- /dev/null +++ b/change/change-2e852706-e348-4268-baee-efe7ed5475ca.json @@ -0,0 +1,11 @@ +{ + "changes": [ + { + "type": "minor", + "comment": "Fix bug where we have unnecessary dependency edges added to the graph when a package does not have the relevant target in scripts", + "packageName": "@lage-run/target-graph", + "email": "1581488+christiango@users.noreply.github.com", + "dependentChangeType": "patch" + } + ] +} \ No newline at end of file diff --git a/packages/e2e-tests/src/transitiveTaskDeps.test.ts b/packages/e2e-tests/src/transitiveTaskDeps.test.ts index a8859aeb2..8185955f4 100644 --- a/packages/e2e-tests/src/transitiveTaskDeps.test.ts +++ b/packages/e2e-tests/src/transitiveTaskDeps.test.ts @@ -207,4 +207,51 @@ describe("transitive task deps test", () => { await repo.cleanup(); }); + + it("reproduce bug where transitive dependencies were being added that were not necessary", async () => { + // Simulates a bug from an internal repo that implemented isolated declarations for some packages + const repo = new Monorepo("transitiveDeps-isolated-declarations-info"); + + await repo.init(); + + // This repo has some packages that have isolated declarations configured and some that do not. + // For the packages that do not have isolatedDeclarations enabled, we have a dummy emitDeclarations task defined for them whose sole purpose is to make sure we block on those package's typecheck step for d.ts emission + // For packages that do have isolatedDeclarations enabled, we emit the d.ts during transpile so we omit the emitDeclarations task. + await repo.setLageConfig(`module.exports = { + pipeline: { + transpile: [], + emitDeclarations: ["typecheck"], + typecheck: ["^^emitDeclarations", "transpile", "^^transpile"] + }, + }`); + + await repo.addPackage("dep", [], { + transpile: "echo dep:transpile", + typecheck: "echo dep:typecheck", + }); + + await repo.addPackage("app", ["dep"], { + transpile: "echo app:transpile", + typecheck: "echo app:typecheck", + emitDeclarations: "echo app:emitDeclarations", + }); + + await repo.install(); + + const results = await repo.run("writeInfo", ["typecheck", "--scope", "app"]); + + const output = results.stdout + results.stderr; + const jsonOutput = parseNdJson(output); + const packageTasks = jsonOutput[0].data.packageTasks; + + const appTypecheckTask = packageTasks.find(({ id }: { id: string }) => id === "app#typecheck"); + expect(appTypecheckTask).toBeTruthy(); + + expect(appTypecheckTask.dependencies).toContain("app#transpile"); + + // This was the bug, we'd end up with app depending on the typecheck step of the dependency which does not have an emitDeclarations step + expect(appTypecheckTask.dependencies).not.toContain("dep#typecheck"); + + await repo.cleanup(); + }); }); diff --git a/packages/target-graph/src/WorkspaceTargetGraphBuilder.ts b/packages/target-graph/src/WorkspaceTargetGraphBuilder.ts index 7c00c9c9a..06af9f33b 100644 --- a/packages/target-graph/src/WorkspaceTargetGraphBuilder.ts +++ b/packages/target-graph/src/WorkspaceTargetGraphBuilder.ts @@ -95,8 +95,10 @@ export class WorkspaceTargetGraphBuilder { this.processStagedConfig(target, config, changedFiles); } else { const packages = Object.keys(this.packageInfos); + for (const packageName of packages) { const task = id; + const targetConfig = this.determineFinalTargetConfig(getTargetId(packageName, task), config); const target = this.targetFactory.createPackageTarget(packageName!, task, targetConfig); this.graphBuilder.addTarget(target); @@ -207,7 +209,7 @@ export class WorkspaceTargetGraphBuilder { priorities?: { package?: string; task: string; priority: number }[] ): Promise { // Expands the dependency specs from the target definitions - const fullDependencies = expandDepSpecs(this.graphBuilder.targets, this.dependencyMap); + const fullDependencies = expandDepSpecs(this.graphBuilder.targets, this.dependencyMap, this.packageInfos); for (const [from, to] of fullDependencies) { this.graphBuilder.addDependency(from, to); diff --git a/packages/target-graph/src/__tests__/WorkspaceTargetGraphBuilder.test.ts b/packages/target-graph/src/__tests__/WorkspaceTargetGraphBuilder.test.ts index 78dc9ecba..82a1fca64 100644 --- a/packages/target-graph/src/__tests__/WorkspaceTargetGraphBuilder.test.ts +++ b/packages/target-graph/src/__tests__/WorkspaceTargetGraphBuilder.test.ts @@ -402,4 +402,77 @@ describe("workspace target graph builder", () => { ] `); }); + + it("should not create phantom transitive deps for packages missing a script", async () => { + const root = "/repos/a"; + + // "app" has emitDeclarations in scripts, "dep" does not + const packageInfos = createPackageInfoWithScripts({ + app: { deps: ["dep"], scripts: ["transpile", "typecheck", "emitDeclarations"] }, + dep: { deps: [], scripts: ["transpile", "typecheck"] }, + }); + + const builder = new WorkspaceTargetGraphBuilder(root, packageInfos, false); + await builder.addTargetConfig("transpile"); + await builder.addTargetConfig("emitDeclarations", { + dependsOn: ["typecheck"], + }); + await builder.addTargetConfig("typecheck", { + dependsOn: ["^^emitDeclarations", "transpile", "^^transpile"], + }); + + const targetGraph = await builder.build(["typecheck"], ["app"]); + const graph = getGraphFromTargets(targetGraph); + + // app#typecheck should depend on app#transpile (same-package dep) + expect(graph).toContainEqual(["app#transpile", "app#typecheck"]); + + // app#typecheck should depend on dep#transpile (via ^^transpile, dep has the script) + expect(graph).toContainEqual(["dep#transpile", "app#typecheck"]); + + // app#typecheck should NOT depend on dep#typecheck — dep doesn't have emitDeclarations, + // so the phantom dep#emitDeclarations should not create a transitive link + expect(graph).not.toContainEqual(["dep#typecheck", "app#typecheck"]); + }); + + it("should preserve ^^ deps for non-npmScript typed targets even without scripts entry", async () => { + const root = "/repos/a"; + + // "dep" doesn't have "customTask" in scripts, but it's configured as a worker type + const packageInfos = createPackageInfoWithScripts({ + app: { deps: ["dep"], scripts: ["build"] }, + dep: { deps: [], scripts: ["build"] }, + }); + + const builder = new WorkspaceTargetGraphBuilder(root, packageInfos, false); + await builder.addTargetConfig("customTask", { + type: "worker", + }); + await builder.addTargetConfig("build", { + dependsOn: ["^^customTask"], + }); + + const targetGraph = await builder.build(["build"], ["app"]); + const graph = getGraphFromTargets(targetGraph); + + // app#build should depend on dep#customTask even though dep doesn't have it in scripts, + // because the target has an explicit non-npmScript type ("worker") + expect(graph).toContainEqual(["dep#customTask", "app#build"]); + }); }); + +function createPackageInfoWithScripts(packages: { [id: string]: { deps: string[]; scripts: string[] } }) { + const packageInfos: PackageInfos = {}; + Object.keys(packages).forEach((id) => { + const { deps, scripts } = packages[id]; + packageInfos[id] = { + packageJsonPath: `/path/to/${id}/package.json`, + name: id, + version: "1.0.0", + dependencies: deps.reduce((acc, dep) => ({ ...acc, [dep]: "*" }), {}), + devDependencies: {}, + scripts: scripts.reduce((acc, script) => ({ ...acc, [script]: `echo ${script}` }), {}), + }; + }); + return packageInfos; +} diff --git a/packages/target-graph/src/expandDepSpecs.ts b/packages/target-graph/src/expandDepSpecs.ts index 2992832c5..83a359c11 100644 --- a/packages/target-graph/src/expandDepSpecs.ts +++ b/packages/target-graph/src/expandDepSpecs.ts @@ -1,11 +1,46 @@ import type { Target } from "./types/Target.js"; import type { DependencyMap } from "workspace-tools/lib/graph/createDependencyMap.js"; +import type { PackageInfos } from "workspace-tools"; import { getPackageAndTask, getStartTargetId, getTargetId } from "./targetId.js"; +/** + * Checks whether a target represents a "phantom" target — one created for a package that + * doesn't actually define the script for an npmScript target. Phantom targets should be excluded from `^` and `^^` + * (topological/transitive) dependency expansion to avoid unwanted cross-package dependency chains. + * + * When phantom targets are later removed by `removeNodes` (because `shouldRun` returns false), + * their same-package dependencies get reconnected to their cross-package dependents, creating + * unnecessary work. + * + * Returns true if the target should be EXCLUDED from dependency expansion. + */ +function isPhantomTarget(targetId: string, task: string, targets: Map, packageInfos?: PackageInfos): boolean { + if (!packageInfos) return false; + + const target = targets.get(targetId); + if (!target?.packageName) return false; + + // Only npmScript targets can be phantom — other types (worker, noop, etc.) + // are real targets regardless of whether the package defines the script. + if (target.type !== "npmScript") { + return false; + } + + const pkgScripts = packageInfos[target.packageName]?.scripts; + // If the package has a scripts section but doesn't include this task, it's a phantom target. + // If the package has no scripts section at all (e.g., in unit tests), we include the target + // for backward compatibility. + return !!pkgScripts && !pkgScripts[task]; +} + /** * Expands the dependency graph by adding all transitive dependencies of the given targets. */ -export function expandDepSpecs(targets: Map, dependencyMap: DependencyMap): [string, string][] { +export function expandDepSpecs( + targets: Map, + dependencyMap: DependencyMap, + packageInfos?: PackageInfos +): [string, string][] { const dependencies: [string, string][] = []; /** @@ -77,6 +112,8 @@ export function expandDepSpecs(targets: Map, dependencyMap: Depe const targetDependencies = [...(getTransitiveGraphDependencies(packageName, dependencyMap) ?? [])]; const dependencyTargetIds = findDependenciesByTask(depTask, targetDependencies); for (const from of dependencyTargetIds) { + // Skip phantom targets: packages that don't define this task as a real npm script. + if (isPhantomTarget(from, depTask, targets, packageInfos)) continue; addDependency(from, to); } } else if (dependencyTargetId.startsWith("^") && packageName) { @@ -85,6 +122,8 @@ export function expandDepSpecs(targets: Map, dependencyMap: Depe const targetDependencies = [...(dependencyMap.dependencies.get(packageName) ?? [])]; const dependencyTargetIds = findDependenciesByTask(depTask, targetDependencies); for (const from of dependencyTargetIds) { + // Skip phantom targets: packages that don't define this task as a real npm script. + if (isPhantomTarget(from, depTask, targets, packageInfos)) continue; addDependency(from, to); } } else if (packageName) {