fix: add missing Collection Observer disconnect method (#1761)

* fix: add missing Collection Observer disconnect method

* fix: add missing collection observer in vanilla grid implementation

examples/vite-demo-vanilla-bundle/src/examples/example07.ts

@@ -602,7 +602,6 @@ export default class Example07 {
     // you can dynamically add your column to your column definitions
     // and then use the spread operator [...cols] OR slice to force the framework to review the changes
     this.sgb.columnDefinitions.push(newCol);
-    this.sgb.columnDefinitions = this.sgb.columnDefinitions.slice(); // or use spread operator [...cols]
 
     // NOTE if you use an Extensions (Checkbox Selector, Row Detail, ...) that modifies the column definitions in any way
     // you MUST use "getAllColumnDefinitions()" from the GridService, using this will be ALL columns including the 1st column that is created internally
@@ -616,7 +615,6 @@ export default class Example07 {
 
   dynamicallyRemoveLastColumn() {
     this.sgb.columnDefinitions.pop();
-    this.sgb.columnDefinitions = this.sgb.columnDefinitions.slice();
 
     /*
     const allColumns = this.slickerGridInstance.gridService.getAllColumnDefinitions();

packages/common/src/filters/autocompleterFilter.ts

@@ -39,6 +39,7 @@ export class AutocompleterFilter<T extends AutocompleteItem = any> implements Fi
   protected _bindEventService: BindingEventService;
   protected _clearFilterTriggered = false;
   protected _collection?: any[];
+  protected _collectionObservers: Array<null | ({ disconnect: () => void; })> = [];
   protected _filterElm!: HTMLInputElement;
   protected _instance: any;
   protected _locales!: Locale;
@@ -243,6 +244,7 @@ export class AutocompleterFilter<T extends AutocompleteItem = any> implements Fi
     this._filterElm?.remove?.();
     this._collection = undefined;
     this._bindEventService.unbindAll();
+    this._collectionObservers.forEach(obs => obs?.disconnect());
 
     // unsubscribe all the possible Observables if RxJS was used
     unsubscribeAll(this.subscriptions);
@@ -316,20 +318,24 @@ export class AutocompleterFilter<T extends AutocompleteItem = any> implements Fi
   protected watchCollectionChanges(): void {
     if (this.columnFilter?.collection) {
       // subscribe to the "collection" changes (array `push`, `unshift`, `splice`, ...)
-      collectionObserver(this.columnFilter.collection, (updatedArray) => {
-        this.renderDomElement(this.columnFilter.collection || updatedArray || []);
-      });
+      this._collectionObservers.push(
+        collectionObserver(this.columnFilter.collection, (updatedArray) => {
+          this.renderDomElement(this.columnFilter.collection || updatedArray || []);
+        })
+      );
 
       // observe for any "collection" changes (array replace)
-      // then simply recreate/re-render the Select (dropdown) DOM Element
+      // then simply recreate/re-render the filter 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 || []);
-          });
+          this._collectionObservers.push(
+            collectionObserver(this.columnFilter.collection, (updatedArray) => {
+              this.renderDomElement(this.columnFilter.collection || updatedArray || []);
+            })
+          );
         }
       });
     }

packages/common/src/filters/selectFilter.ts

@@ -24,6 +24,7 @@ import type { SlickGrid } from '../core/index.js';
 export class SelectFilter implements Filter {
   protected _isMultipleSelect = true;
   protected _collectionLength = 0;
+  protected _collectionObservers: Array<null | ({ disconnect: () => void; })> = [];
   protected _locales!: Locale;
   protected _msInstance?: MultipleSelectInstance;
   protected _shouldTriggerQuery = true;
@@ -216,6 +217,9 @@ export class SelectFilter implements Filter {
     }
     this.filterElm?.remove();
 
+    // TODO: causing Example 7 E2E tests to fails, will revisit later
+    // this._collectionObservers.forEach(obs => obs?.disconnect());
+
     // unsubscribe all the possible Observables if RxJS was used
     unsubscribeAll(this.subscriptions);
   }
@@ -295,7 +299,9 @@ export class SelectFilter implements Filter {
   protected watchCollectionChanges(): void {
     if (this.columnFilter?.collection) {
       // subscribe to the "collection" changes (array `push`, `unshift`, `splice`, ...)
-      collectionObserver(this.columnFilter.collection, this.watchCallback.bind(this));
+      this._collectionObservers.push(
+        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
@@ -308,7 +314,9 @@ export class SelectFilter implements Filter {
 
     // 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));
+      this._collectionObservers.push(
+        collectionObserver(this.columnFilter.collection, this.watchCallback.bind(this))
+      );
     }
   }
 

packages/common/src/services/__tests__/observers.spec.ts

@@ -86,6 +86,29 @@ describe('Service/Observers', () => {
       });
       inputArray.sort((obj1, obj2) => obj1.id - obj2.id);
     }));
+
+    it('should disconnect observer and expect callback to no longer be executed', () => {
+      let callbackCount = 0;
+      const collection = [1, 2];
+      const observer = collectionObserver(collection, () => callbackCount++);
+      collection.push(Math.random());
+      collection.push(Math.random());
+
+      expect(collection.length).toBe(4);
+
+      observer?.disconnect();
+
+      collection.push(Math.random());
+      collection.push(Math.random());
+
+      expect(collection.length).toBe(4);
+    });
+
+    it('should return null when input is not an array type', () => {
+      const observer = collectionObserver('text' as any, () => console.log('hello'));
+
+      expect(observer).toBeNull();
+    });
   });
 
   describe('propertyObserver method', () => {

packages/common/src/services/index.ts

@@ -11,6 +11,7 @@ export * from './grid.service.js';
 export * from './gridEvent.service.js';
 export * from './gridState.service.js';
 export * from './headerGrouping.service.js';
+export * from './observers.js';
 export * from './pagination.service.js';
 export * from './resizer.service.js';
 export * from './rxjsFacade.js';

packages/common/src/services/observers.ts

@@ -1,20 +1,33 @@
 /**
  * Collection Observer to watch for any array changes (pop, push, reverse, shift, unshift, splice, sort)
  * and execute the callback when any of the methods are called
- * @param {any[]} inputArray - array you want to listen to
+ * @param {any[]} arr - array you want to listen to
  * @param {Function} callback function that will be called on any change inside array
  */
-export function collectionObserver(inputArray: any[], callback: (outputArray: any[], newValues: any[]) => void): void {
-  // Add more methods here if you want to listen to them
-  const mutationMethods = ['pop', 'push', 'reverse', 'shift', 'unshift', 'splice', 'sort'];
+export function collectionObserver(arr: any[], callback: (outputArray: any[], newValues: any[]) => void): null | ({ disconnect: () => void; }) {
+  if (Array.isArray(arr)) {
+    // Add more methods here if you want to listen to them
+    const mutationMethods = ['pop', 'push', 'reverse', 'shift', 'unshift', 'splice', 'sort'];
+    const originalActions: Array<{ method: string; action: () => void; }> = [];
 
-  mutationMethods.forEach((changeMethod: any) => {
-    inputArray[changeMethod] = (...args: any[]) => {
-      const res = Array.prototype[changeMethod].apply(inputArray, args);  // call normal behaviour
-      callback.apply(inputArray, [inputArray, args]);  // finally call the callback supplied
-      return res;
-    };
-  });
+    mutationMethods.forEach((changeMethod: any) => {
+      arr[changeMethod] = (...args: any[]) => {
+        const res = Array.prototype[changeMethod].apply(arr, args);  // call normal behaviour
+        originalActions.push({ method: changeMethod, action: res });
+        callback.apply(arr, [arr, args]);  // finally call the callback supplied
+        return res;
+      };
+    });
+
+    // reapply original behaviors when disconnecting
+    const disconnect = () => mutationMethods.forEach((changeMethod: any) => {
+      arr[changeMethod] = () => originalActions[changeMethod].action;
+    });
+
+    return { disconnect };
+  }
+
+  return null;
 }
 
 /**

packages/vanilla-bundle/src/components/__tests__/slick-vanilla-grid.spec.ts

@@ -472,6 +472,19 @@ describe('Slick-Vanilla-Grid-Bundle Component instantiated via Constructor', ()
     expect(component.columnDefinitions).toEqual(columnsMock);
   });
 
+  it('should update call updateColumnDefinitionsList() when pushing to the column definitions', () => {
+    const updateColumnsSpy = vi.spyOn(component, 'updateColumnDefinitionsList');
+    component.initialization(divContainer, slickEventHandler);
+
+    const newCol = { id: 'hobbies', name: 'Hobbies', field: 'hobbie' };
+    component.columnDefinitions.push(newCol);
+
+    expect(updateColumnsSpy).toHaveBeenCalled();
+    expect(component.columnDefinitions).toEqual(
+      expect.arrayContaining([{ id: 'name', field: 'name' }, newCol])
+    );
+  });
+
   describe('initialization method', () => {
     const customEditableInputFormatter: Formatter = (_row, _cell, value, columnDef) => {
       const isEditableItem = !!columnDef.editor;
@@ -561,7 +574,10 @@ describe('Slick-Vanilla-Grid-Bundle Component instantiated via Constructor', ()
         component.gridOptions = { autoAddCustomEditorFormatter: customEditableInputFormatter };
         component.initialization(divContainer, slickEventHandler);
 
-        expect(autoAddEditorFormatterToColumnsWithEditor).toHaveBeenCalledWith([{ id: 'name', field: 'name', editor: undefined }], customEditableInputFormatter);
+        expect(autoAddEditorFormatterToColumnsWithEditor).toHaveBeenCalledWith(
+          expect.arrayContaining([{ id: 'name', field: 'name' }]),
+          customEditableInputFormatter
+        );
       });
     });
 

packages/vanilla-bundle/src/components/slick-vanilla-grid-bundle.ts

@@ -27,6 +27,7 @@ import {
   // services
   BackendUtilityService,
   CollectionService,
+  collectionObserver,
   ExtensionService,
   ExtensionUtility,
   FilterFactory,
@@ -67,6 +68,7 @@ export class SlickVanillaGridBundle<TData = any> {
   protected _currentDatasetLength = 0;
   protected _eventPubSubService!: EventPubSubService;
   protected _darkMode = false;
+  protected _collectionObservers: Array<null | ({ disconnect: () => void; })> = [];
   protected _columnDefinitions?: Column<TData>[];
   protected _gridOptions?: GridOption;
   protected _gridContainerElm!: HTMLElement;
@@ -478,6 +480,7 @@ export class SlickVanillaGridBundle<TData = any> {
     if (shouldEmptyDomElementContainer) {
       this.emptyGridContainerElm();
     }
+    this._collectionObservers.forEach(obs => obs?.disconnect());
     this._eventPubSubService?.dispose();
     this._slickerGridInstances = null as any;
   }
@@ -712,6 +715,9 @@ export class SlickVanillaGridBundle<TData = any> {
     this._eventPubSubService.publish('onSlickerGridCreated', this.instances);
     this._isGridInitialized = true;
     this.suggestDateParsingWhenHelpful();
+
+    // subscribe to column definitions assignment changes
+    this.observeColumnDefinitions();
   }
 
   hasBackendInfiniteScroll(): boolean {
@@ -1254,6 +1260,27 @@ export class SlickVanillaGridBundle<TData = any> {
     }
   }
 
+  /** handler for when column definitions changes */
+  protected columnDefinitionsHandler(): void {
+    this._columnDefinitions = this.columnDefinitions;
+    if (this._isGridInitialized) {
+      this.updateColumnDefinitionsList(this.columnDefinitions);
+    }
+    if (this._columnDefinitions.length > 0) {
+      this.copyColumnWidthsReference(this._columnDefinitions);
+    }
+  }
+
+  /**
+   * assignment changes are not triggering on the column definitions, for that
+   * we can use our internal array observer for any changes done via (push, pop, shift, ...)
+   */
+  protected observeColumnDefinitions(): void {
+    this._collectionObservers.push(
+      collectionObserver(this.columnDefinitions, this.columnDefinitionsHandler.bind(this))
+    );
+  }
+
   /**
    * Render (or dispose) the Pagination Component, user can optionally provide False (to not show it) which will in term dispose of the Pagination,
    * also while disposing we can choose to omit the disposable of the Pagination Service (if we are simply toggling the Pagination, we want to keep the Service alive)