Deprecate getNodeKey method on IPresentationTreeDataProvider (#120)

* Deprecate `getNodeKey` method

* Change files

* Fixes

change/@itwin-presentation-components-f8848e73-3fd0-4360-9047-5222eb408513.json

@@ -0,0 +1,7 @@
+{
+  "type": "minor",
+  "comment": "`IPresentationTreeDataProvider`: deprecated `getNodeKey` method in favor of getting `NodeKey` directly from `TreeNodeItem` using `isPresentationTreeNodeItem` typeguard.",
+  "packageName": "@itwin/presentation-components",
+  "email": "24278440+saskliutas@users.noreply.github.com",
+  "dependentChangeType": "patch"
+}

packages/components/src/presentation-components/tree/DataProvider.ts

@@ -12,8 +12,8 @@ import { DelayLoadedTreeNodeItem, PageOptions, TreeNodeItem } from "@itwin/compo
 import { IDisposable, Logger } from "@itwin/core-bentley";
 import { IModelConnection } from "@itwin/core-frontend";
 import {
-  ClientDiagnosticsOptions, FilterByTextHierarchyRequestOptions, HierarchyRequestOptions, InstanceFilterDefinition, Node, NodeKey, NodePathElement,
-  Paged, PresentationError, PresentationStatus, RequestOptionsWithRuleset, Ruleset,
+  BaseNodeKey, ClientDiagnosticsOptions, FilterByTextHierarchyRequestOptions, HierarchyRequestOptions, InstanceFilterDefinition, Node, NodeKey,
+  NodePathElement, Paged, PresentationError, PresentationStatus, RequestOptionsWithRuleset, Ruleset,
 } from "@itwin/presentation-common";
 import { Presentation } from "@itwin/presentation-frontend";
 import { createDiagnosticsOptions, DiagnosticsProps } from "../common/Diagnostics";
@@ -183,10 +183,14 @@ export class PresentationTreeDataProvider implements IPresentationTreeDataProvid
 
   /**
    * Returns a [NodeKey]($presentation-common) from given [TreeNodeItem]($components-react).
-   * **Warning:** the `node` must be created by this data provider.
+   *
+   * **Warning**: Returns invalid [NodeKey]($presentation-common) if `node` is not a [[PresentationTreeNodeItem]].
+   *
+   * @deprecated in 4.0. Use [[isPresentationTreeNodeItem]] and [[PresentationTreeNodeItem.key]] to get [NodeKey]($presentation-common).
    */
   public getNodeKey(node: TreeNodeItem): NodeKey {
-    return (node as PresentationTreeNodeItem).key;
+    const invalidKey: BaseNodeKey = { type: "", pathFromRoot: [], version: 0 };
+    return isPresentationTreeNodeItem(node) ? node.key : invalidKey;
   }
 
   /**
@@ -214,7 +218,7 @@ export class PresentationTreeDataProvider implements IPresentationTreeDataProvid
   }
 
   private _getNodesAndCount = memoize(async (parentNode?: TreeNodeItem, pageOptions?: PageOptions, instanceFilter?: InstanceFilterDefinition): Promise<{ nodes: TreeNodeItem[], count: number }> => {
-    const parentKey = parentNode ? this.getNodeKey(parentNode) : undefined;
+    const parentKey = parentNode && isPresentationTreeNodeItem(parentNode) ? parentNode.key : undefined;
     const requestOptions = this.createRequestOptions(parentKey, pageOptions, instanceFilter);
     return createNodesAndCountResult(async () => this._dataSource.getNodesAndCount(requestOptions), this.createBaseRequestOptions(), parentNode, this._nodesCreateProps);
   }, { isMatchingKey: MemoizationHelpers.areNodesRequestsEqual as any });

packages/components/src/presentation-components/tree/FilteredDataProvider.ts

@@ -141,7 +141,9 @@ export class FilteredPresentationTreeDataProvider implements IFilteredPresentati
     return this._parentDataProvider.getFilteredNodePaths(filter);
   }
 
+  /** @deprecated in 4.0. Use [[isPresentationTreeNodeItem]] and [[PresentationTreeNodeItem.key]] to get [NodeKey]($presentation-common). */
   public getNodeKey(node: TreeNodeItem): NodeKey {
+    // eslint-disable-next-line deprecation/deprecation
     return this._parentDataProvider.getNodeKey(node);
   }
 

packages/components/src/presentation-components/tree/IPresentationTreeDataProvider.ts

@@ -17,6 +17,8 @@ import { IPresentationDataProvider } from "../common/IPresentationDataProvider";
 export interface IPresentationTreeDataProvider extends ITreeDataProvider, IPresentationDataProvider {
   /**
    * Returns a [NodeKey]($presentation-common) from given [TreeNodeItem]($components-react).
+   *
+   * @deprecated in 4.0. Use [[isPresentationTreeNodeItem]] and [[PresentationTreeNodeItem.key]] to get [NodeKey]($presentation-common).
    */
   getNodeKey(node: TreeNodeItem): NodeKey;
 

packages/components/src/presentation-components/tree/controlled/UseUnifiedSelection.ts

@@ -19,6 +19,7 @@ import { useDisposable } from "@itwin/core-react";
 import { Keys, KeySet, NodeKey } from "@itwin/presentation-common";
 import { Presentation, SelectionChangeEventArgs, SelectionChangeType, SelectionHandler, SelectionHelper } from "@itwin/presentation-frontend";
 import { IPresentationTreeDataProvider } from "../IPresentationTreeDataProvider";
+import { isPresentationTreeNodeItem } from "../PresentationTreeNodeItem";
 import { toRxjsObservable } from "../Utils";
 
 /**
@@ -142,7 +143,10 @@ export class UnifiedSelectionTreeEventHandler extends TreeEventHandler implement
       this.updateAllNodes(selection);
   }
 
-  protected getNodeKey(node: TreeNodeItem) {
+  /** @deprecated in 4.0. Use [[isPresentationTreeNodeItem]] and [[PresentationTreeNodeItem.key]] to get [NodeKey]($presentation-common). */
+  // istanbul ignore next
+  protected getNodeKey(node: TreeNodeItem): NodeKey {
+    // eslint-disable-next-line deprecation/deprecation
     return this._dataProvider.getNodeKey(node);
   }
 
@@ -152,7 +156,7 @@ export class UnifiedSelectionTreeEventHandler extends TreeEventHandler implement
    * or node is ECInstance node and instance key is in selection.
    */
   protected shouldSelectNode(node: TreeNodeItem, selection: Readonly<KeySet>) {
-    const nodeKey = this.getNodeKey(node);
+    const nodeKey = isPresentationTreeNodeItem(node) ? node.key : undefined;
     if (nodeKey === undefined)
       return false;
 
@@ -176,7 +180,9 @@ export class UnifiedSelectionTreeEventHandler extends TreeEventHandler implement
   }
 
   protected getKeys(nodes: TreeNodeItem[]): Keys {
-    const nodeKeys: NodeKey[] = nodes.map((node) => this._dataProvider.getNodeKey(node));
+    const nodeKeys: NodeKey[] = nodes
+      .map((node) => isPresentationTreeNodeItem(node) ? node.key : /* istanbul ignore next */ undefined)
+      .filter((key) => key !== undefined) as NodeKey[];
     return SelectionHelper.getKeysForSelection(nodeKeys);
   }
 

packages/components/src/test/tree/DataProvider.test.ts

@@ -7,6 +7,7 @@ import { expect } from "chai";
 import equal from "fast-deep-equal";
 import * as sinon from "sinon";
 import * as moq from "typemoq";
+import { PropertyRecord } from "@itwin/appui-abstract";
 import { PageOptions, PropertyFilterRuleOperator } from "@itwin/components-react";
 import { BeEvent, Logger } from "@itwin/core-bentley";
 import { EmptyLocalization } from "@itwin/core-common";
@@ -104,6 +105,27 @@ describe("TreeDataProvider", () => {
 
   });
 
+  describe("getNodeKey", () => {
+
+    it("returns invalid key for non presentation tree node item", () => {
+      const key = provider.getNodeKey({ id: "test_id", label: PropertyRecord.fromString("Test Label") }); // eslint-disable-line deprecation/deprecation
+      expect(key.type).to.be.empty;
+      expect(key.pathFromRoot).to.be.empty;
+      expect(key.version).to.be.eq(0);
+    });
+
+    it("returns valid key for presentation tree node item", () => {
+      const nodeKey = createTestECInstancesNodeKey();
+      const item: PresentationTreeNodeItem = {
+        id: "test_id",
+        label: PropertyRecord.fromString("Test Label"),
+        key: nodeKey,
+      };
+      expect(provider.getNodeKey(item)).to.be.eq(nodeKey); // eslint-disable-line deprecation/deprecation
+    });
+
+  });
+
   describe("getNodesCount", () => {
 
     it("returns presentation manager result through `getNodesAndCount`", async () => {

packages/components/src/test/tree/FilteredDataProvider.test.ts

@@ -172,8 +172,8 @@ describe("FilteredTreeDataProvider", () => {
       const key = createTestECInstancesNodeKey();
       const treeNode = createTestTreeNodeItem(key);
 
-      parentProviderMock.setup((x) => x.getNodeKey(treeNode)).returns(() => key);
-      const result = provider.getNodeKey(treeNode);
+      parentProviderMock.setup((x) => x.getNodeKey(treeNode)).returns(() => key); // eslint-disable-line deprecation/deprecation
+      const result = provider.getNodeKey(treeNode); // eslint-disable-line deprecation/deprecation
       expect(result).to.deep.equal(key);
     });
 

packages/components/src/test/tree/controlled/UseUnifiedSelection.test.ts

@@ -43,9 +43,6 @@ describe("UnifiedSelectionEventHandler", () => {
   const selectionHandlerMock = moq.Mock.ofType<SelectionHandler>();
   const treeModelMock = moq.Mock.ofType<TreeModel>();
   const dataProviderMock = moq.Mock.ofType<IPresentationTreeDataProvider>();
-  dataProviderMock.setup((x) => x.getNodeKey(moq.It.isAny())).returns((n: TreeNodeItem) => {
-    return (n as PresentationTreeNodeItem).key;
-  });
 
   let onModelChangeEvent: BeUiEvent<[TreeModel, TreeModelChanges]>;
 
@@ -91,6 +88,8 @@ describe("UnifiedSelectionEventHandler", () => {
     return node;
   };
 
+  const getItemKey = (item: TreeNodeItem) => (item as PresentationTreeNodeItem).key;
+
   describe("modelSource", () => {
 
     it("returns modelSource", () => {
@@ -105,8 +104,8 @@ describe("UnifiedSelectionEventHandler", () => {
       const node1: MutableTreeModelNode = createNode();
       const node2: MutableTreeModelNode = createNode(createTestECClassGroupingNodeKey);
       const selectionKeys = SelectionHelper.getKeysForSelection([
-        dataProviderMock.target.getNodeKey(node1.item),
-        dataProviderMock.target.getNodeKey(node2.item),
+        getItemKey(node1.item),
+        getItemKey(node2.item),
       ]);
 
       treeModelSourceMock.setup((x) => x.getModel()).returns(() => treeModelMock.object);
@@ -128,8 +127,8 @@ describe("UnifiedSelectionEventHandler", () => {
       const node1: MutableTreeModelNode = createNode();
       const node2: MutableTreeModelNode = createNode(createTestECClassGroupingNodeKey);
       const selectionKeys = SelectionHelper.getKeysForSelection([
-        dataProviderMock.target.getNodeKey(node1.item),
-        dataProviderMock.target.getNodeKey(node2.item),
+        getItemKey(node1.item),
+        getItemKey(node2.item),
       ]);
 
       treeModelSourceMock.setup((x) => x.getModel()).returns(() => treeModelMock.object);
@@ -182,8 +181,8 @@ describe("UnifiedSelectionEventHandler", () => {
       const node1: MutableTreeModelNode = createNode();
       const node2: MutableTreeModelNode = createNode(createTestECClassGroupingNodeKey);
       const selectionKeys = SelectionHelper.getKeysForSelection([
-        dataProviderMock.target.getNodeKey(node1.item),
-        dataProviderMock.target.getNodeKey(node2.item),
+        getItemKey(node1.item),
+        getItemKey(node2.item),
       ]);
 
       treeModelSourceMock.setup((x) => x.getModel()).returns(() => treeModelMock.object);
@@ -215,8 +214,8 @@ describe("UnifiedSelectionEventHandler", () => {
       unifiedEventHandler.onSelectionReplaced(event);
       await waitForCompletion();
 
-      selectionHandlerMock.verify((x) => x.replaceSelection(SelectionHelper.getKeysForSelection([dataProviderMock.target.getNodeKey(node1.item)])), moq.Times.once());
-      selectionHandlerMock.verify((x) => x.addToSelection(SelectionHelper.getKeysForSelection([dataProviderMock.target.getNodeKey(node2.item)])), moq.Times.once());
+      selectionHandlerMock.verify((x) => x.replaceSelection(SelectionHelper.getKeysForSelection([getItemKey(node1.item)])), moq.Times.once());
+      selectionHandlerMock.verify((x) => x.addToSelection(SelectionHelper.getKeysForSelection([getItemKey(node2.item)])), moq.Times.once());
     });
 
     it("does not replace selection if event does not have nodes", async () => {
@@ -265,7 +264,7 @@ describe("UnifiedSelectionEventHandler", () => {
 
     it("applies unified selection for added nodes", () => {
       const node = createNode();
-      selectionHandlerMock.setup((x) => x.getSelection()).returns(() => new KeySet([dataProviderMock.target.getNodeKey(node.item)]));
+      selectionHandlerMock.setup((x) => x.getSelection()).returns(() => new KeySet([getItemKey(node.item)]));
       treeModelSourceMock.setup((x) => x.modifyModel(moq.It.isAny())).callback((action) => action(treeModelMock.object));
       treeModelMock.setup((x) => x.getNode(node.id)).returns(() => node);
 
@@ -275,7 +274,7 @@ describe("UnifiedSelectionEventHandler", () => {
 
     it("applies unified selection for modified nodes", () => {
       const node = createNode();
-      selectionHandlerMock.setup((x) => x.getSelection()).returns(() => new KeySet([dataProviderMock.target.getNodeKey(node.item)]));
+      selectionHandlerMock.setup((x) => x.getSelection()).returns(() => new KeySet([getItemKey(node.item)]));
       treeModelSourceMock.setup((x) => x.modifyModel(moq.It.isAny())).callback((action) => action(treeModelMock.object));
       treeModelMock.setup((x) => x.getNode(node.id)).returns(() => node);
 
@@ -286,7 +285,7 @@ describe("UnifiedSelectionEventHandler", () => {
     it("deselects added node without key", () => {
       const node = createNode();
       node.isSelected = true;
-      const keySet = new KeySet([dataProviderMock.target.getNodeKey(node.item)]);
+      const keySet = new KeySet([getItemKey(node.item)]);
       selectionHandlerMock.setup((x) => x.getSelection()).returns(() => keySet);
       treeModelSourceMock.setup((x) => x.modifyModel(moq.It.isAny())).callback((action) => action(treeModelMock.object));
       (node.item as Partial<PresentationTreeNodeItem>).key = undefined;
@@ -332,7 +331,7 @@ describe("UnifiedSelectionEventHandler", () => {
         createNode(createTestECClassGroupingNodeKey),
       ];
       nodes[1].isSelected = true;
-      const selectionKeys = SelectionHelper.getKeysForSelection(nodes.map((n) => dataProviderMock.target.getNodeKey(n.item)));
+      const selectionKeys = SelectionHelper.getKeysForSelection(nodes.map((n) => getItemKey(n.item)));
 
       selectionHandlerMock.setup((x) => x.getSelection()).returns(() => new KeySet(selectionKeys));
       treeModelMock.setup((x) => x.iterateTreeModelNodes()).returns(() => nodes[Symbol.iterator]());

packages/full-stack-tests/src/components/tree/TreeDataProvider.test.ts

@@ -9,7 +9,7 @@ import { PropertyValueFormat } from "@itwin/appui-abstract";
 import { assert, Guid } from "@itwin/core-bentley";
 import { IModelConnection, SnapshotConnection } from "@itwin/core-frontend";
 import { ChildNodeSpecificationTypes, NodeKey, Ruleset, RuleTypes } from "@itwin/presentation-common";
-import { isPresentationInfoTreeNodeItem, PresentationTreeDataProvider } from "@itwin/presentation-components";
+import { isPresentationInfoTreeNodeItem, PresentationTreeDataProvider, PresentationTreeNodeItem } from "@itwin/presentation-components";
 import { Presentation } from "@itwin/presentation-frontend";
 import { initialize, terminate } from "../../IntegrationTests";
 
@@ -159,7 +159,7 @@ describe("TreeDataProvider", async () => {
     const nodes = await provider.getNodes(undefined);
     expect(nodes).to.not.be.empty;
     nodes.forEach((item) => {
-      const key = provider.getNodeKey(item);
+      const key = (item as PresentationTreeNodeItem).key;
       assert(NodeKey.isClassGroupingNodeKey(key));
       assert(item.label.value.valueFormat === PropertyValueFormat.Primitive);
       expect(item.label.value.displayValue).to.match(new RegExp(`^[\\w\\d_ ]+ \\(${key.groupedInstancesCount}\\)$`, "i"));