fix: DraggableGrouping & Select Filter collectionAsync mem leaks (#…

…1247) * fix: DraggableGrouping & Select Filter `collectionAsync` mem leaks
ghiscoding · Dec 4, 2023 · 7dcf53a · 7dcf53a
1 parent 79d86fe
commit 7dcf53a
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 34 deletions.
diff --git a/packages/common/src/extensions/slickDraggableGrouping.ts b/packages/common/src/extensions/slickDraggableGrouping.ts
@@ -215,6 +215,9 @@ export class SlickDraggableGrouping {
   /** Dispose the plugin. */
   dispose() {
     this.destroySortableInstances();
+    if (this._droppableInstance?.el) {
+      this._droppableInstance?.destroy();
+    }
     this.onGroupChanged.unsubscribe();
     this._eventHandler.unsubscribeAll();
     this.pubSubService.unsubscribeAll(this._subscriptions);

diff --git a/packages/common/src/filters/__tests__/selectFilter.spec.ts b/packages/common/src/filters/__tests__/selectFilter.spec.ts
@@ -86,23 +86,11 @@ describe('SelectFilter', () => {
   });
 
   it('should throw an error when collection is not a valid array', (done) => {
-    try {
-      mockColumn.filter!.collection = { hello: 'world' } as any;
-      filter.init(filterArguments);
-    } catch (e) {
+    mockColumn.filter!.collection = { hello: 'world' } as any;
+    filter.init(filterArguments).catch(e => {
       expect(e.message).toContain(`The "collection" passed to the Select Filter is not a valid array.`);
       done();
-    }
-  });
-
-  it('should throw an error when collection is not a valid value/label pair array', (done) => {
-    try {
-      mockColumn.filter!.collection = [{ hello: 'world' }];
-      filter.init(filterArguments);
-    } catch (e) {
-      expect(e.message).toContain(`[Slickgrid-Universal] Select Filter/Editor collection with value/label (or value/labelKey when using Locale) is required to populate the Select list`);
-      done();
-    }
+    });
   });
 
   it('should throw an error when "enableTranslateLabel" is set without a valid I18N Service', (done) => {

diff --git a/packages/common/src/filters/selectFilter.ts b/packages/common/src/filters/selectFilter.ts
@@ -163,27 +163,26 @@ export class SelectFilter implements Filter {
     // always render the Select (dropdown) DOM element,
     // if that is the case, the Select will simply be without any options but we still have to render it (else SlickGrid would throw an error)
     const newCollection = this.columnFilter.collection || [];
-    this.renderDomElement(newCollection);
 
     return new Promise(async (resolve, reject) => {
       try {
-        const collectionAsync = this.columnFilter.collectionAsync;
         let collectionOutput: Promise<any[]> | any[] | undefined;
 
-        if (collectionAsync && !this.columnFilter.collection) {
+        if (this.columnFilter.collectionAsync && !this.columnFilter.collection) {
           // only read the collectionAsync once (on the 1st load),
           // we do this because Http Fetch will throw an error saying body was already read and its streaming is locked
-          collectionOutput = renderCollectionOptionsAsync(collectionAsync, this.columnDef, this.renderDomElement.bind(this), this.rxjs, this.subscriptions);
+          collectionOutput = renderCollectionOptionsAsync(this.columnFilter.collectionAsync, this.columnDef, this.renderDomElement.bind(this), this.rxjs, this.subscriptions);
           resolve(collectionOutput);
         } else {
           collectionOutput = newCollection;
+          this.renderDomElement(newCollection);
           resolve(newCollection);
         }
 
         // subscribe to both CollectionObserver and PropertyObserver
         // any collection changes will trigger a re-render of the DOM element filter
-        if (collectionAsync || this.columnFilter.enableCollectionWatch) {
-          await (collectionOutput ?? collectionAsync);
+        if (this.columnFilter.collectionAsync || this.columnFilter.enableCollectionWatch) {
+          await (collectionOutput ?? this.columnFilter.collectionAsync);
           this.watchCollectionChanges();
         }
       } catch (e) {
@@ -286,25 +285,27 @@ export class SelectFilter implements Filter {
   protected watchCollectionChanges() {
     if (this.columnFilter?.collection) {
       // subscribe to the "collection" changes (array `push`, `unshift`, `splice`, ...)
-      collectionObserver(this.columnFilter.collection, (updatedArray) => {
-        this.renderDomElement(this.columnFilter.collection || updatedArray || []);
-      });
+      collectionObserver(this.columnFilter.collection, this.watchCallback.bind(this));
 
       // observe for any "collection" changes (array replace)
       // then simply recreate/re-render the Select (dropdown) DOM Element
-      propertyObserver(this.columnFilter, 'collection', (newValue) => {
-        this.renderDomElement(newValue || []);
-
-        // when new assignment arrives, we need to also reassign observer to the new reference
-        if (this.columnFilter.collection) {
-          collectionObserver(this.columnFilter.collection, (updatedArray) => {
-            this.renderDomElement(this.columnFilter.collection || updatedArray || []);
-          });
-        }
-      });
+      propertyObserver(this.columnFilter, 'collection', this.propertyObserverCallback.bind(this));
     }
   }
 
+  protected propertyObserverCallback(newValue: any) {
+    this.renderDomElement(newValue || []);
+
+    // when new assignment arrives, we need to also reassign observer to the new reference
+    if (this.columnFilter.collection) {
+      collectionObserver(this.columnFilter.collection, this.watchCallback.bind(this));
+    }
+  }
+
+  protected watchCallback(updatedArray: any[]) {
+    this.renderDomElement(this.columnFilter.collection || updatedArray || []);
+  }
+
   renderDomElement(inputCollection: any[]) {
     if (!Array.isArray(inputCollection) && this.collectionOptions?.collectionInsideObjectProperty) {
       const collectionInsideObjectProperty = this.collectionOptions.collectionInsideObjectProperty;