refactor(workspace): make env-DAG layering recursive

scopes/workspace/workspace/workspace-component/env-dag-sort.spec.ts

@@ -6,75 +6,101 @@ 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()]);
     const envOf: Record<string, string | undefined> = {
       [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');
     const envOf: Record<string, string> = {
       [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<string, string> = {
+      [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<string, string> = {
+      [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);
   });
 });

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<string>
-): [ComponentID[], ComponentID[]] {
-  const envsOfEnvs = new Set<string>();
-  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<string, ComponentID>();
+  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<string, ComponentID | null>();
   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<string, number>();
+  const inProgress = new Set<string>();
+  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<number, ComponentID[]>();
+  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;
 }

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<string>): Array<ComponentID[]> {
     return groupEnvsByDepLayer(