diff --git a/change/change-811de810-7c2d-4e6c-96b7-c671b4d7ffcb.json b/change/change-811de810-7c2d-4e6c-96b7-c671b4d7ffcb.json new file mode 100644 index 000000000..d923c849f --- /dev/null +++ b/change/change-811de810-7c2d-4e6c-96b7-c671b4d7ffcb.json @@ -0,0 +1,25 @@ +{ + "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/cli", + "email": "1581488+christiango@users.noreply.github.com", + "dependentChangeType": "patch" + }, + { + "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/config", + "email": "1581488+christiango@users.noreply.github.com", + "dependentChangeType": "patch" + }, + { + "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/change/change-af1026b1-217e-47f5-9bf6-56d614cb46c9.json b/change/change-af1026b1-217e-47f5-9bf6-56d614cb46c9.json deleted file mode 100644 index 53de9432f..000000000 --- a/change/change-af1026b1-217e-47f5-9bf6-56d614cb46c9.json +++ /dev/null @@ -1,32 +0,0 @@ -{ - "changes": [ - { - "type": "patch", - "comment": "Update backfill-cache to 5.11.3", - "packageName": "@lage-run/cache", - "email": "elcraig@microsoft.com", - "dependentChangeType": "patch" - }, - { - "type": "patch", - "comment": "Update config generated by init", - "packageName": "@lage-run/cli", - "email": "elcraig@microsoft.com", - "dependentChangeType": "patch" - }, - { - "type": "patch", - "comment": "Reuse utilities from backfill-hasher", - "packageName": "@lage-run/hasher", - "email": "elcraig@microsoft.com", - "dependentChangeType": "patch" - }, - { - "type": "none", - "comment": "Update readme", - "packageName": "lage", - "email": "elcraig@microsoft.com", - "dependentChangeType": "none" - } - ] -} \ No newline at end of file diff --git a/packages/cli/src/__tests__/createTargetGraph.test.ts b/packages/cli/src/__tests__/createTargetGraph.test.ts index 6527d87f6..79bddbc70 100644 --- a/packages/cli/src/__tests__/createTargetGraph.test.ts +++ b/packages/cli/src/__tests__/createTargetGraph.test.ts @@ -129,6 +129,7 @@ async function createPackageTasks(tasks: string[], packageInfos: PackageInfos, p dependencies: options.dependencies, dependents: options.dependents && !options.to, // --to is a short hand for --scope + --no-dependents enableTargetConfigMerging: true, + enablePhantomTargetOptimization: false, ignore: options.ignore.concat(config.ignore), pipeline: config.pipeline, repoWideChanges: config.repoWideChanges, diff --git a/packages/cli/src/commands/info/action.ts b/packages/cli/src/commands/info/action.ts index ee1f26bdb..be93ba465 100644 --- a/packages/cli/src/commands/info/action.ts +++ b/packages/cli/src/commands/info/action.ts @@ -123,6 +123,7 @@ export async function infoAction(options: InfoActionOptions, command: Command): packageInfos, priorities: config.priorities, enableTargetConfigMerging: config.enableTargetConfigMerging, + enablePhantomTargetOptimization: config.enablePhantomTargetOptimization, }); const scope = getFilteredPackages({ diff --git a/packages/cli/src/commands/run/createTargetGraph.ts b/packages/cli/src/commands/run/createTargetGraph.ts index ebfbcef67..49b76e007 100644 --- a/packages/cli/src/commands/run/createTargetGraph.ts +++ b/packages/cli/src/commands/run/createTargetGraph.ts @@ -21,6 +21,7 @@ interface CreateTargetGraphOptions { packageInfos: PackageInfos; priorities: Priority[]; enableTargetConfigMerging: boolean; + enablePhantomTargetOptimization: boolean; } function getChangedFiles(since: string, cwd: string) { @@ -45,6 +46,7 @@ export async function createTargetGraph(options: CreateTargetGraphOptions): Prom dependencies, dependents, enableTargetConfigMerging, + enablePhantomTargetOptimization, since, scope, repoWideChanges, @@ -56,7 +58,7 @@ export async function createTargetGraph(options: CreateTargetGraphOptions): Prom priorities, } = options; - const builder = new WorkspaceTargetGraphBuilder(root, packageInfos, enableTargetConfigMerging); + const builder = new WorkspaceTargetGraphBuilder(root, packageInfos, enableTargetConfigMerging, enablePhantomTargetOptimization); const packages = getFilteredPackages({ root, diff --git a/packages/cli/src/commands/run/runAction.ts b/packages/cli/src/commands/run/runAction.ts index 822034e9d..d465d42c2 100644 --- a/packages/cli/src/commands/run/runAction.ts +++ b/packages/cli/src/commands/run/runAction.ts @@ -65,6 +65,7 @@ export async function runAction(options: RunOptions, command: Command): Promise< packageInfos, priorities: config.priorities, enableTargetConfigMerging: config.enableTargetConfigMerging, + enablePhantomTargetOptimization: config.enablePhantomTargetOptimization, }); validateTargetGraph(targetGraph, allowNoTargetRuns); diff --git a/packages/cli/src/commands/run/watchAction.ts b/packages/cli/src/commands/run/watchAction.ts index 900a27229..98f37e19a 100644 --- a/packages/cli/src/commands/run/watchAction.ts +++ b/packages/cli/src/commands/run/watchAction.ts @@ -63,6 +63,7 @@ export async function watchAction(options: RunOptions, command: Command): Promis packageInfos, priorities: config.priorities, enableTargetConfigMerging: config.enableTargetConfigMerging, + enablePhantomTargetOptimization: config.enablePhantomTargetOptimization, }); // Make sure we do not attempt writeRemoteCache in watch mode diff --git a/packages/cli/src/commands/server/lageService.ts b/packages/cli/src/commands/server/lageService.ts index 9b2e47091..a06363c1d 100644 --- a/packages/cli/src/commands/server/lageService.ts +++ b/packages/cli/src/commands/server/lageService.ts @@ -78,6 +78,7 @@ async function createInitializedPromise({ cwd, logger, serverControls, nodeArg, packageInfos, priorities: config.priorities, enableTargetConfigMerging: config.enableTargetConfigMerging, + enablePhantomTargetOptimization: config.enablePhantomTargetOptimization, }); const targetHasher = new TargetHasher({ diff --git a/packages/config/src/getConfig.ts b/packages/config/src/getConfig.ts index c7e112502..ef175ed66 100644 --- a/packages/config/src/getConfig.ts +++ b/packages/config/src/getConfig.ts @@ -31,6 +31,7 @@ export async function getConfig(cwd: string): Promise { concurrency: config?.concurrency ?? availableParallelism, allowNoTargetRuns: config?.allowNoTargetRuns ?? false, enableTargetConfigMerging: config?.enableTargetConfigMerging ?? false, + enablePhantomTargetOptimization: config?.enablePhantomTargetOptimization ?? false, reporters: config?.reporters ?? {}, }; } diff --git a/packages/config/src/types/ConfigOptions.ts b/packages/config/src/types/ConfigOptions.ts index 38dd85021..ba9268127 100644 --- a/packages/config/src/types/ConfigOptions.ts +++ b/packages/config/src/types/ConfigOptions.ts @@ -84,6 +84,13 @@ export interface ConfigOptions { */ enableTargetConfigMerging: boolean; + /** + * Enables phantom target optimization. When enabled, targets created for packages that don't actually define + * the corresponding npm script are excluded from transitive (`^^`) dependency expansion. + * This prevents unwanted cross-package dependency chains when phantom targets are later removed. + */ + enablePhantomTargetOptimization: boolean; + /** * Custom reporters that can be referenced by name in the --reporter CLI flag. * The key is the reporter name, and the value is the path to the reporter module. 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..620dfd06d 100644 --- a/packages/target-graph/src/WorkspaceTargetGraphBuilder.ts +++ b/packages/target-graph/src/WorkspaceTargetGraphBuilder.ts @@ -54,7 +54,8 @@ export class WorkspaceTargetGraphBuilder { constructor( root: string, private packageInfos: PackageInfos, - private enableTargetConfigMerging: boolean + private enableTargetConfigMerging: boolean, + private enablePhantomTargetOptimization: boolean ) { this.dependencyMap = createDependencyMap(packageInfos, { withDevDependencies: true, withPeerDependencies: false }); this.graphBuilder = new TargetGraphBuilder(); @@ -95,8 +96,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 +210,12 @@ 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, + this.enablePhantomTargetOptimization + ); 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..38cb72219 100644 --- a/packages/target-graph/src/__tests__/WorkspaceTargetGraphBuilder.test.ts +++ b/packages/target-graph/src/__tests__/WorkspaceTargetGraphBuilder.test.ts @@ -38,7 +38,7 @@ describe("workspace target graph builder", () => { b: [], }); - const builder = new WorkspaceTargetGraphBuilder(root, packageInfos, false); + const builder = new WorkspaceTargetGraphBuilder(root, packageInfos, false, false); await builder.addTargetConfig("build", { dependsOn: ["^build"], }); @@ -80,7 +80,7 @@ describe("workspace target graph builder", () => { b: [], }); - const builder = new WorkspaceTargetGraphBuilder(root, packageInfos, false); + const builder = new WorkspaceTargetGraphBuilder(root, packageInfos, false, false); await builder.addTargetConfig("test"); await builder.addTargetConfig("lint"); @@ -119,7 +119,7 @@ describe("workspace target graph builder", () => { c: ["b"], }); - const builder = new WorkspaceTargetGraphBuilder(root, packageInfos, false); + const builder = new WorkspaceTargetGraphBuilder(root, packageInfos, false, false); await builder.addTargetConfig("build", { dependsOn: ["^build"], @@ -161,7 +161,7 @@ describe("workspace target graph builder", () => { c: ["b"], }); - const builder = new WorkspaceTargetGraphBuilder(root, packageInfos, false); + const builder = new WorkspaceTargetGraphBuilder(root, packageInfos, false, false); await builder.addTargetConfig("build", { dependsOn: ["^build"], @@ -195,7 +195,7 @@ describe("workspace target graph builder", () => { c: [], }); - const builder = new WorkspaceTargetGraphBuilder(root, packageInfos, false); + const builder = new WorkspaceTargetGraphBuilder(root, packageInfos, false, false); await builder.addTargetConfig("bundle", { dependsOn: ["^^transpile"], @@ -240,7 +240,7 @@ describe("workspace target graph builder", () => { common: [], }); - const builder = new WorkspaceTargetGraphBuilder(root, packageInfos, false); + const builder = new WorkspaceTargetGraphBuilder(root, packageInfos, false, false); await builder.addTargetConfig("build", { dependsOn: ["common#copy", "^build"], @@ -296,7 +296,7 @@ describe("workspace target graph builder", () => { b: [], }); - const builder = new WorkspaceTargetGraphBuilder(root, packageInfos, false); + const builder = new WorkspaceTargetGraphBuilder(root, packageInfos, false, false); await builder.addTargetConfig("build", { dependsOn: ["^build", "#global:task"], }); @@ -345,7 +345,7 @@ describe("workspace target graph builder", () => { b: [], }); - const builder = new WorkspaceTargetGraphBuilder(root, packageInfos, false); + const builder = new WorkspaceTargetGraphBuilder(root, packageInfos, false, false); await builder.addTargetConfig("build", { dependsOn: ["^build", "#global:task"], }); @@ -374,7 +374,7 @@ describe("workspace target graph builder", () => { b: [], }); - const builder = new WorkspaceTargetGraphBuilder(root, packageInfos, false); + const builder = new WorkspaceTargetGraphBuilder(root, packageInfos, false, false); await builder.addTargetConfig("build", { dependsOn: ["^build"], }); @@ -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, 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, 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..35af19c2c 100644 --- a/packages/target-graph/src/expandDepSpecs.ts +++ b/packages/target-graph/src/expandDepSpecs.ts @@ -1,11 +1,47 @@ 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. + * + * Only npmScript target types can be considered phantom targets + * + * Returns true if the target should be EXCLUDED from dependency expansion. + */ +function isPhantomTarget(targetId: string, task: string, targets: Map, packageInfos: PackageInfos): boolean { + 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, + enablePhantomTargetOptimization: boolean +): [string, string][] { const dependencies: [string, string][] = []; /** @@ -77,6 +113,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 (enablePhantomTargetOptimization && isPhantomTarget(from, depTask, targets, packageInfos)) continue; addDependency(from, to); } } else if (dependencyTargetId.startsWith("^") && packageName) { @@ -85,6 +123,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 (enablePhantomTargetOptimization && isPhantomTarget(from, depTask, targets, packageInfos)) continue; addDependency(from, to); } } else if (packageName) {