Hierarchy builder: Fix determine children operator (#322)

* Fix "DetermineChildren" operator possibly ruining order of the nodes * changeset
iTwin · Oct 20, 2023 · fd0575f · fd0575f
1 parent b70fa9e
commit fd0575f
Show file tree

Hide file tree

Showing 4 changed files with 52 additions and 23 deletions.
diff --git a/.changeset/tame-dancers-occur.md b/.changeset/tame-dancers-occur.md
@@ -0,0 +1,2 @@
+---
+---
diff --git a/packages/full-stack-tests/src/hierarchy-builder/grouping/MultiLevelGrouping.test.ts b/packages/full-stack-tests/src/hierarchy-builder/grouping/MultiLevelGrouping.test.ts
@@ -114,6 +114,10 @@ describe("Stateless hierarchy builder", () => {
               NodeValidators.createForClassGroupingNode({
                 className: physicalPartitionClassName,
                 children: [
+                  NodeValidators.createForInstanceNode({
+                    instanceKeys: [keys.childPartition3],
+                    children: false,
+                  }),
                   NodeValidators.createForLabelGroupingNode({
                     label: labelGroupName2,
                     children: [
@@ -127,10 +131,6 @@ describe("Stateless hierarchy builder", () => {
                       }),
                     ],
                   }),
-                  NodeValidators.createForInstanceNode({
-                    instanceKeys: [keys.childPartition3],
-                    children: false,
-                  }),
                 ],
               }),
               NodeValidators.createForClassGroupingNode({

diff --git a/packages/hierarchy-builder/src/hierarchy-builder/internal/operators/DetermineChildren.ts b/packages/hierarchy-builder/src/hierarchy-builder/internal/operators/DetermineChildren.ts
@@ -3,10 +3,10 @@
  * See LICENSE.md in the project root for license terms and full copyright notice.
  *--------------------------------------------------------------------------------------------*/
 
-import { map, merge, mergeMap, Observable, partition, shareReplay, tap } from "rxjs";
+import { concatMap, map, Observable, of, tap } from "rxjs";
 import { HierarchyNode } from "../../HierarchyNode";
 import { getLogger } from "../../Logging";
-import { createOperatorLoggingNamespace, hasChildren } from "../Common";
+import { createOperatorLoggingNamespace } from "../Common";
 
 const OPERATOR_NAME = "DetermineChildren";
 /** @internal */
@@ -19,24 +19,19 @@ export const LOGGING_NAMESPACE = createOperatorLoggingNamespace(OPERATOR_NAME);
  */
 export function createDetermineChildrenOperator(hasNodes: (node: HierarchyNode) => Observable<boolean>) {
   return function (nodes: Observable<HierarchyNode>): Observable<HierarchyNode> {
-    const sharedNodes = nodes.pipe(
+    return nodes.pipe(
       log((n) => `in: ${n.label}`),
-      // each partitioned observable is going to subscribe to this individually - share and replay to avoid requesting
-      // nodes from source observable multiple times
-      shareReplay(),
+      concatMap((n: HierarchyNode): Observable<HierarchyNode> => {
+        if (n.children !== undefined) {
+          return of(n);
+        }
+        return hasNodes(n).pipe(
+          log((hasChildrenFlag) => `determined children for ${n.label}: ${hasChildrenFlag}`),
+          map((hasChildrenFlag) => ({ ...n, children: hasChildrenFlag })),
+        );
+      }),
+      log((n) => `out: ${n.label}`),
     );
-    const [determined, undetermined] = partition(sharedNodes, (node) => node.children !== undefined);
-    return merge(
-      determined,
-      undetermined.pipe(
-        mergeMap((n) =>
-          hasNodes(n).pipe(
-            log((hasChildrenFlag) => `children for ${n.label}: ${hasChildrenFlag}`),
-            map((hasChildrenFlag) => ({ ...n, children: hasChildrenFlag })),
-          ),
-        ),
-      ),
-    ).pipe(log((n) => `out: ${n.label} / ${hasChildren(n)}`));
   };
 }
 

diff --git a/packages/hierarchy-builder/src/test/internal/operators/DetermineChildren.test.ts b/packages/hierarchy-builder/src/test/internal/operators/DetermineChildren.test.ts
@@ -4,7 +4,7 @@
  *--------------------------------------------------------------------------------------------*/
 
 import { expect } from "chai";
-import { from, of } from "rxjs";
+import { delay, from, Observable, of } from "rxjs";
 import sinon from "sinon";
 import { LogLevel } from "@itwin/core-bentley";
 import { HierarchyNode } from "../../../hierarchy-builder/HierarchyNode";
@@ -39,4 +39,36 @@ describe("DetermineChildren", () => {
     expect(hasNodes).to.be.calledOnceWith(node);
     expect(result).to.deep.eq([{ ...node, children: true }]);
   });
+
+  it("streams nodes in the same order as input", async () => {
+    const nodes: HierarchyNode[] = [
+      // will determine children of this node asynchronously
+      {
+        key: "1",
+        label: "1",
+        children: undefined,
+      },
+      // will determine children of this node synchronously
+      {
+        key: "2",
+        label: "2",
+        children: undefined,
+      },
+      // this node already has children determined
+      {
+        key: "3",
+        label: "3",
+        children: true,
+      },
+    ];
+    const hasNodes = (parent: HierarchyNode): Observable<boolean> => {
+      const res = of(false);
+      if (parent.key === "1") {
+        return res.pipe(delay(1));
+      }
+      return res;
+    };
+    const result = await getObservableResult(from(nodes).pipe(createDetermineChildrenOperator(hasNodes)));
+    expect(result.map((n) => n.key)).to.deep.eq(["1", "2", "3"]);
+  });
 });