[NG] Stop caching children for lazy recursive trees

We used to fetch children for lazy recursive trees only once, but we closed their selection Observable when we shouldn't. Rather than avoid closing the Observables when we reuse models, I decided to stop caching children altogether, letting the app decide whether they want to cache or not and do it on their side. It's less intrusive and more flexible.

Fixes #3209.

Signed-off-by: Eudes Petonnet-Vincent <epetonnetvince@vmware.com>

src/clr-angular/data/tree-view/models/recursive-tree-node.model.spec.ts

@@ -41,6 +41,19 @@ export default function(): void {
       expect(nbFetch).toBe(1);
     });
 
+    it('offers a clearChildren() method that forces the children to be fetched again next time', function() {
+      let nbFetch = 0;
+      const root = new RecursiveTreeNodeModel('A', null, node => {
+        nbFetch++;
+        return synchronousChildren(node);
+      });
+      root.fetchChildren();
+      expect(nbFetch).toBe(1);
+      root.clearChildren();
+      root.fetchChildren();
+      expect(nbFetch).toBe(2);
+    });
+
     it('declares itself as parent for created children', function() {
       const root = new RecursiveTreeNodeModel('A', null, synchronousChildren);
       expect(root.children.map(c => c.parent)).toEqual([root, root]);

src/clr-angular/data/tree-view/models/recursive-tree-node.model.ts

@@ -28,6 +28,12 @@ export class RecursiveTreeNodeModel<T> extends TreeNodeModel<T> {
 
   private childrenFetched = false;
 
+  clearChildren() {
+    this._children.forEach(child => child.destroy());
+    delete this._children;
+    this.childrenFetched = false;
+  }
+
   fetchChildren() {
     if (this.childrenFetched) {
       return;

src/clr-angular/data/tree-view/recursive-children.spec.ts

@@ -62,6 +62,11 @@ export default function(): void {
     beforeEach(function(this: Context) {
       this.featuresService = this.getProvider<TreeFeaturesService<TestNode>>(TreeFeaturesService);
       this.expandService = this.getProvider(Expand);
+      this.featuresService.recursion = {
+        template: this.hostComponent.template,
+        root: [TEST_ROOT],
+      };
+      this.detectChanges();
     });
 
     it('can create a ClrRecursiveForOfContext for a node', function(this: Context) {
@@ -72,50 +77,52 @@ export default function(): void {
     });
 
     it('does not render anything if the tree is not recursive', function(this: Context) {
+      delete this.featuresService.recursion;
+      this.detectChanges();
       expect(this.clarityElement.textContent.trim()).toBe('');
     });
 
     it('renders children if the tree is eager', function(this: Context) {
-      this.featuresService.recursion = {
-        template: this.hostComponent.template,
-        root: [TEST_ROOT],
-      };
-      this.detectChanges();
       expect(this.clarityElement.textContent).toMatch(/A\s*B\s*C/);
     });
 
     it('does not render anything if the tree is lazy and the parent is collapsed', function(this: Context) {
-      this.featuresService.recursion = {
-        template: this.hostComponent.template,
-        root: [TEST_ROOT],
-      };
       this.featuresService.eager = false;
       this.detectChanges();
       expect(this.clarityElement.textContent.trim()).toBe('');
     });
 
     it('renders children if the tree is lazy and the parent is expanded', function(this: Context) {
-      this.featuresService.recursion = {
-        template: this.hostComponent.template,
-        root: [TEST_ROOT],
-      };
       this.featuresService.eager = false;
       this.expandService.expanded = true;
       this.detectChanges();
       expect(this.clarityElement.textContent).toMatch(/A\s*B\s*C/);
     });
 
     it('renders children even if there is no parent', function(this: Context) {
-      this.featuresService.recursion = {
-        template: this.hostComponent.template,
-        root: [TEST_ROOT],
-      };
       delete this.hostComponent.parent;
       this.hostComponent.children = TEST_ROOT.children;
       this.detectChanges();
       expect(this.clarityElement.textContent).toMatch(/A\s*B\s*C/);
     });
 
+    it('does not destroy the children when he parent becomes collapsed if the tree is eager', function(this: Context) {
+      const spy = spyOn(this.hostComponent.parent, 'clearChildren');
+      this.expandService.expanded = true;
+      expect(spy).not.toHaveBeenCalled();
+      this.expandService.expanded = false;
+      expect(spy).not.toHaveBeenCalled();
+    });
+
+    it('destroys the children when he parent becomes collapsed if the tree is lazy', function(this: Context) {
+      this.featuresService.eager = false;
+      const spy = spyOn(this.hostComponent.parent, 'clearChildren');
+      this.expandService.expanded = true;
+      expect(spy).not.toHaveBeenCalled();
+      this.expandService.expanded = false;
+      expect(spy).toHaveBeenCalled();
+    });
+
     // Just like other specs, this is not nearly as exhaustive as I would like it to be due to time constraints.
   });
 }

src/clr-angular/data/tree-view/recursive-children.ts

@@ -29,13 +29,13 @@ import { RecursiveTreeNodeModel } from './models/recursive-tree-node.model';
  */
 export class RecursiveChildren<T> {
   constructor(public featuresService: TreeFeaturesService<T>, @Optional() private expandService: Expand) {
-    if (expandService && featuresService.recursion) {
+    if (expandService) {
       this.subscription = this.expandService.expandChange.subscribe(value => {
-        if (value && this.parent) {
-          // Once again, I'm sure we can find a way to avoid this casting by typing every component in a way that
-          // lets us use the more specific model classes depending on the use of *clrRecursiveForOf or not.
-          // But it would take time, which we don't have right now.
-          (<RecursiveTreeNodeModel<T>>this.parent).fetchChildren();
+        if (!value && this.parent && !this.featuresService.eager && this.featuresService.recursion) {
+          // In the case of lazy-loading recursive trees, we clear the children on collapse.
+          // This is better in case they change between two user interaction, and that way
+          // the app itself can decide whether to cache them or not.
+          (<RecursiveTreeNodeModel<T>>this.parent).clearChildren();
         }
       });
     }