From 36e47348fcb89d5089b0ec56fb297f77c685e814 Mon Sep 17 00:00:00 2001 From: David First Date: Thu, 7 May 2026 10:57:28 -0400 Subject: [PATCH] refactor(workspace): make env-DAG layering recursive Replaces V1's one-level partition with a proper topological sort over the env-of-env DAG. Cycles fall back to a single layer defensively. Per D-001, this is the load-order primitive the rewrite depends on. --- .../workspace-component/env-dag-sort.spec.ts | 82 ++++++++----- .../workspace-component/env-dag-sort.ts | 115 +++++++++++++----- .../workspace-component-loader.ts | 3 +- 3 files changed, 139 insertions(+), 61 deletions(-) diff --git a/scopes/workspace/workspace/workspace-component/env-dag-sort.spec.ts b/scopes/workspace/workspace/workspace-component/env-dag-sort.spec.ts index 4f7f4f691272..86d6afddf24b 100644 --- a/scopes/workspace/workspace/workspace-component/env-dag-sort.spec.ts +++ b/scopes/workspace/workspace/workspace-component/env-dag-sort.spec.ts @@ -6,48 +6,50 @@ function id(idStr: string): ComponentID { return ComponentID.fromString(idStr); } +function withoutVersion(c: ComponentID): string { + return c.toStringWithoutVersion(); +} + describe('groupEnvsByDepLayer', () => { - it('returns both layers empty for empty input', () => { - const [deeper, shallower] = groupEnvsByDepLayer([], () => undefined, new Set()); - expect(deeper).to.deep.equal([]); - expect(shallower).to.deep.equal([]); + it('returns an empty array for empty input', () => { + expect(groupEnvsByDepLayer([], () => undefined, new Set())).to.deep.equal([]); }); - it('puts every env in shallower when no env-of-env relationships exist in the list', () => { + it('returns a single layer when no env-of-env relationships exist in the list', () => { const ids = [id('teambit.react/react@1.0.0'), id('teambit.node/node@1.0.0')]; - const [deeper, shallower] = groupEnvsByDepLayer(ids, () => undefined, new Set()); - expect(deeper).to.deep.equal([]); - expect(shallower).to.have.lengthOf(2); + const layers = groupEnvsByDepLayer(ids, () => undefined, new Set()); + expect(layers).to.have.lengthOf(1); + expect(layers[0]).to.have.lengthOf(2); }); it('layers correctly when one env is the env of another env in the list', () => { // ReactEnv's env is BitEnv. Both are in the list. BitEnv must load first. const reactEnv = id('teambit.react/react@1.0.0'); const bitEnv = id('bitdev.general/envs/bit-env@1.0.0'); - const [deeper, shallower] = groupEnvsByDepLayer( + const layers = groupEnvsByDepLayer( [reactEnv, bitEnv], (envId) => (envId.toString() === reactEnv.toString() ? bitEnv.toStringWithoutVersion() : undefined), new Set() ); - expect(deeper.map((c) => c.toStringWithoutVersion())).to.deep.equal(['bitdev.general/envs/bit-env']); - expect(shallower.map((c) => c.toStringWithoutVersion())).to.deep.equal(['teambit.react/react']); + expect(layers).to.have.lengthOf(2); + expect(layers[0].map(withoutVersion)).to.deep.equal(['bitdev.general/envs/bit-env']); + expect(layers[1].map(withoutVersion)).to.deep.equal(['teambit.react/react']); }); it('matches env-of-env by id without version', () => { - // The lookup returns an id-without-version; we must still match the versioned id in the list. const reactEnv = id('teambit.react/react@1.0.0'); const bitEnv = id('bitdev.general/envs/bit-env@2.5.0'); - const [deeper] = groupEnvsByDepLayer( + const layers = groupEnvsByDepLayer( [reactEnv, bitEnv], (envId) => (envId.toString() === reactEnv.toString() ? 'bitdev.general/envs/bit-env' : undefined), new Set() ); - expect(deeper).to.have.lengthOf(1); - expect(deeper[0].toString()).to.equal(bitEnv.toString()); + expect(layers).to.have.lengthOf(2); + expect(layers[0]).to.have.lengthOf(1); + expect(layers[0][0].toString()).to.equal(bitEnv.toString()); }); it('skips propagating env-of-env when the env is itself a workspace-component env', () => { - // Quirk preserved from V1's regroupEnvsIdsFromTheList. See env-dag-sort.ts. const reactEnv = id('teambit.react/react@1.0.0'); const bitEnv = id('bitdev.general/envs/bit-env@1.0.0'); const wsEnvs = new Set([reactEnv.toString()]); @@ -55,16 +57,14 @@ describe('groupEnvsByDepLayer', () => { [reactEnv.toString()]: bitEnv.toStringWithoutVersion(), [bitEnv.toString()]: undefined, }; - const [deeper, shallower] = groupEnvsByDepLayer([reactEnv, bitEnv], (envId) => envOf[envId.toString()], wsEnvs); - // Because reactEnv is in wsEnvs, the reactEnv → bitEnv edge is NOT propagated - // into envsOfEnvs. Nothing else points at bitEnv. Both envs end up in shallower. - expect(deeper).to.deep.equal([]); - expect(shallower).to.have.lengthOf(2); + const layers = groupEnvsByDepLayer([reactEnv, bitEnv], (envId) => envOf[envId.toString()], wsEnvs); + // The reactEnv → bitEnv edge is NOT propagated, so they aren't ordered. + expect(layers).to.have.lengthOf(1); + expect(layers[0]).to.have.lengthOf(2); }); - it('does NOT recursively layer env-of-env-of-env (one-level limitation, by design)', () => { - // This test pins V1's known limitation. When we make the sort recursive in a follow-up, - // this test should fail and be updated. + it('recursively layers env-of-env-of-env', () => { + // A → B → C: load C, then B, then A. const a = id('scope/a@1.0.0'); const b = id('scope/b@1.0.0'); const c = id('scope/c@1.0.0'); @@ -72,9 +72,35 @@ describe('groupEnvsByDepLayer', () => { [a.toString()]: b.toStringWithoutVersion(), [b.toString()]: c.toStringWithoutVersion(), }; - const [deeper, shallower] = groupEnvsByDepLayer([a, b, c], (envId) => envOf[envId.toString()], new Set()); - // Old behavior: deeper = {b, c}, shallower = {a}. We don't get [[c], [b], [a]]. - expect(deeper.map((x) => x.toStringWithoutVersion()).sort()).to.deep.equal(['scope/b', 'scope/c']); - expect(shallower.map((x) => x.toStringWithoutVersion())).to.deep.equal(['scope/a']); + const layers = groupEnvsByDepLayer([a, b, c], (envId) => envOf[envId.toString()], new Set()); + expect(layers.map((layer) => layer.map(withoutVersion))).to.deep.equal([['scope/c'], ['scope/b'], ['scope/a']]); + }); + + it('layers diamond shapes correctly (two envs share an env-of-env)', () => { + // A → C, B → C: C loads first, then [A, B] together. + const a = id('scope/a@1.0.0'); + const b = id('scope/b@1.0.0'); + const c = id('scope/c@1.0.0'); + const envOf: Record = { + [a.toString()]: c.toStringWithoutVersion(), + [b.toString()]: c.toStringWithoutVersion(), + }; + const layers = groupEnvsByDepLayer([a, b, c], (envId) => envOf[envId.toString()], new Set()); + expect(layers).to.have.lengthOf(2); + expect(layers[0].map(withoutVersion)).to.deep.equal(['scope/c']); + expect(layers[1].map(withoutVersion).sort()).to.deep.equal(['scope/a', 'scope/b']); + }); + + it('falls back to a single layer if a cycle is detected', () => { + // A → B → A. Should not infinite-loop. Returns [[A, B]] defensively. + const a = id('scope/a@1.0.0'); + const b = id('scope/b@1.0.0'); + const envOf: Record = { + [a.toString()]: b.toStringWithoutVersion(), + [b.toString()]: a.toStringWithoutVersion(), + }; + const layers = groupEnvsByDepLayer([a, b], (envId) => envOf[envId.toString()], new Set()); + expect(layers).to.have.lengthOf(1); + expect(layers[0]).to.have.lengthOf(2); }); }); diff --git a/scopes/workspace/workspace/workspace-component/env-dag-sort.ts b/scopes/workspace/workspace/workspace-component/env-dag-sort.ts index c1ba5f8948f2..b6b6d2d3175d 100644 --- a/scopes/workspace/workspace/workspace-component/env-dag-sort.ts +++ b/scopes/workspace/workspace/workspace-component/env-dag-sort.ts @@ -1,51 +1,104 @@ import type { ComponentID } from '@teambit/component-id'; /** - * Group env IDs into layers ordered by env-of-env depth. + * Topologically layer env IDs by env-of-env depth. * - * Returns `[deeper, shallower]`: - * - `deeper` — envs that are referenced as the env-of-env by some other env in - * the input list. They must load first because envs that depend on them load next. - * - `shallower` — envs not referenced as env-of-env by anyone else in the list. + * Returns layers in load order: layer 0 is "deepest" (no env-of-env among the + * input list) and must load first; layer N is "shallowest" and loads last. * - * **Behavior-preserving extraction** of the algorithm previously inlined as - * `WorkspaceComponentLoader.regroupEnvsIdsFromTheList`. Quirks preserved: + * Example: + * envIds: [ReactEnv, BitEnv] + * ReactEnv's env is BitEnv, BitEnv's env is not in the list + * → [[BitEnv], [ReactEnv]] (load BitEnv first) * - * 1. Only one level of layering (envs of envs of envs is *not* handled — the - * deeper grouping isn't recursively split). See D-001 in - * `docs/rfcs/component-loading-rewrite/DECISIONS.md` for why and the plan - * to fix it. - * 2. The `envsIdsOfWsComps` filter: when an env in the input list is itself - * used directly by a workspace component, we don't propagate its - * env-of-env relationship into the deeper-layer set. The original intent - * is unclear from history; preserved as-is to avoid behavioral drift. - * Document the rationale before the next refactor touches it. + * envIds: [A, B, C] + * A's env is B, B's env is C, C's env is not in the list + * → [[C], [B], [A]] (load C, then B, then A) + * + * envIds: [A, B] with no env-of-env relationships in the list + * → [[A, B]] (single layer) + * + * Quirk preserved from V1's `regroupEnvsIdsFromTheList`: when an env in the + * input list is itself used directly by a workspace component + * (`envsIdsOfWsComps`), its env-of-env edge is *not* propagated. The original + * intent is unclear from history; preserved as-is to avoid behavioral drift. + * Document the rationale before the next refactor touches it. + * + * Cycles are not expected in real env DAGs. If one is detected, the function + * returns all envs in a single layer (defensive — better than infinite recursion). * * @param envIds the envs to layer (typically, envs used by workspace components) * @param getEnvOfEnv lookup: given an env, return the string id of ITS env (or undefined) * @param envsIdsOfWsComps set of env id strings used directly by workspace components - * @returns `[deeper, shallower]` — load `deeper` first + * @returns layers in load order — `result[0]` loads first */ export function groupEnvsByDepLayer( envIds: ComponentID[], getEnvOfEnv: (id: ComponentID) => string | undefined, envsIdsOfWsComps: Set -): [ComponentID[], ComponentID[]] { - const envsOfEnvs = new Set(); - for (const envId of envIds) { - const idStr = envId.toString(); - if (envsIdsOfWsComps.has(idStr)) continue; - const envOfEnvId = getEnvOfEnv(envId); - if (envOfEnvId) envsOfEnvs.add(envOfEnvId); +): ComponentID[][] { + if (envIds.length === 0) return []; + + // Index input by both with-version and without-version, since lookups in V1 + // matched both forms. + const idIndex = new Map(); + for (const id of envIds) { + idIndex.set(id.toString(), id); + idIndex.set(id.toStringWithoutVersion(), id); } - const deeper: ComponentID[] = []; - const shallower: ComponentID[] = []; + + // For each env, its env-of-env *if that env is also in the input list*. Otherwise null. + const dependsOn = new Map(); for (const id of envIds) { - if (envsOfEnvs.has(id.toString()) || envsOfEnvs.has(id.toStringWithoutVersion())) { - deeper.push(id); - } else { - shallower.push(id); + const idStr = id.toString(); + if (envsIdsOfWsComps.has(idStr)) { + // Quirk: skip propagating the env-of-env edge when this env is a ws-comp env. + dependsOn.set(idStr, null); + continue; + } + const envOfEnvId = getEnvOfEnv(id); + const target = envOfEnvId ? idIndex.get(envOfEnvId) : undefined; + dependsOn.set(idStr, target ?? null); + } + + // Compute depth via memoized DFS, with cycle detection. + const depthCache = new Map(); + const inProgress = new Set(); + let cycleDetected = false; + + function depth(id: ComponentID): number { + const idStr = id.toString(); + if (depthCache.has(idStr)) return depthCache.get(idStr)!; + if (inProgress.has(idStr)) { + cycleDetected = true; + return 0; } + inProgress.add(idStr); + const dep = dependsOn.get(idStr); + const d = dep ? 1 + depth(dep) : 0; + inProgress.delete(idStr); + depthCache.set(idStr, d); + return d; + } + + for (const id of envIds) depth(id); + if (cycleDetected) return [envIds]; + + // Group by depth. + const byDepth = new Map(); + let maxDepth = 0; + for (const id of envIds) { + const d = depthCache.get(id.toString())!; + if (d > maxDepth) maxDepth = d; + if (!byDepth.has(d)) byDepth.set(d, []); + byDepth.get(d)!.push(id); + } + + // Emit ascending: depth 0 (deepest, must load first) → depth maxDepth (loads last). + const layers: ComponentID[][] = []; + for (let d = 0; d <= maxDepth; d++) { + const layer = byDepth.get(d); + if (layer && layer.length > 0) layers.push(layer); } - return [deeper, shallower]; + return layers; } diff --git a/scopes/workspace/workspace/workspace-component/workspace-component-loader.ts b/scopes/workspace/workspace/workspace-component/workspace-component-loader.ts index 88cc99e4b004..9b29635ad244 100644 --- a/scopes/workspace/workspace/workspace-component/workspace-component-loader.ts +++ b/scopes/workspace/workspace/workspace-component/workspace-component-loader.ts @@ -343,8 +343,7 @@ export class WorkspaceComponentLoader { /** * Layer envs so that envs-of-envs load before their dependents. - * Thin wrapper over `groupEnvsByDepLayer` — see that function for the algorithm - * and the one-level-only limitation. + * Thin wrapper over `groupEnvsByDepLayer` — see that function for the algorithm. */ private regroupEnvsIdsFromTheList(envIds: ComponentID[] = [], envsIdsOfWsComps: Set): Array { return groupEnvsByDepLayer(