From 5cae0863276c8d5b5a99d0b383ce10c9f1ae0dd2 Mon Sep 17 00:00:00 2001 From: Tangent Lin <> Date: Tue, 19 Mar 2024 04:32:22 -0400 Subject: [PATCH] fix: PrimaryKeyCollection not include idIndex when buildIndex is called Summary: Test Plan: --- jest.config.js | 1 + package.json | 2 +- src/collections/CollectionViewBase.ts | 53 +++++++-- src/collections/IndexedCollectionBase.ts | 47 ++++++-- src/collections/PrimaryKeyCollection.ts | 15 +++ src/collections/util.ts | 41 +++++++ src/core/ICollectionChangeDetail.ts | 10 ++ src/signals/CollectionAddSignal.ts | 12 ++ src/signals/CollectionChangeSignal.ts | 8 +- src/signals/CollectionRemoveSignal.ts | 12 ++ src/signals/CollectionUpdateSignal.ts | 13 +++ src/signals/index.ts | 3 + test/CollectionSignal.test.ts | 138 +++++++++++++++++++++++ test/collectionview.test.ts | 58 +++++----- test/shared/collections.ts | 38 +++++++ 15 files changed, 394 insertions(+), 57 deletions(-) create mode 100644 src/collections/util.ts create mode 100644 src/core/ICollectionChangeDetail.ts create mode 100644 src/signals/CollectionAddSignal.ts create mode 100644 src/signals/CollectionRemoveSignal.ts create mode 100644 src/signals/CollectionUpdateSignal.ts create mode 100644 test/CollectionSignal.test.ts diff --git a/jest.config.js b/jest.config.js index 3c7d86e..3fa9961 100644 --- a/jest.config.js +++ b/jest.config.js @@ -5,5 +5,6 @@ module.exports = { collectCoverage: true, verbose: true, coverageReporters: ['cobertura', 'html', 'lcov'], + coveragePathIgnorePatterns: ['index.ts$'], testTimeout: 500, }; diff --git a/package.json b/package.json index e9e2558..1c7ca86 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "indexed-collection", - "version": "1.6.0", + "version": "1.8.0", "description": "A zero-dependency library of classes that make filtering, sorting and observing changes to arrays easier and more efficient.", "license": "MIT", "keywords": [ diff --git a/src/collections/CollectionViewBase.ts b/src/collections/CollectionViewBase.ts index 7ae295a..95c919d 100644 --- a/src/collections/CollectionViewBase.ts +++ b/src/collections/CollectionViewBase.ts @@ -1,13 +1,17 @@ +import { ICollectionAndView } from '../core/ICollectionAndView'; +import { ICollectionViewOption } from '../core/ICollectionViewOption'; import { IReadonlyCollection } from '../core/IReadonlyCollection'; import { Optional } from '../core/Optional'; -import { CollectionChangeSignal } from '../signals/CollectionChangeSignal'; -import { SignalObserver } from '../signals/SignalObserver'; -import { ICollectionViewOption } from '../core/ICollectionViewOption'; import { - defaultSort, defaultCollectionViewOption, + defaultSort, } from '../core/defaultCollectionViewOption'; -import { ICollectionAndView } from '../core/ICollectionAndView'; +import { CollectionAddSignal } from '../signals/CollectionAddSignal'; +import { CollectionChangeSignal } from '../signals/CollectionChangeSignal'; +import { CollectionRemoveSignal } from '../signals/CollectionRemoveSignal'; +import { CollectionUpdateSignal } from '../signals/CollectionUpdateSignal'; +import { SignalObserver } from '../signals/SignalObserver'; +import { filterCollectionChangeDetail } from './util'; /** * CollectionView is a view onto a collection of data. Most common use case would be @@ -15,9 +19,12 @@ import { ICollectionAndView } from '../core/ICollectionAndView'; * without modifying the underlying data. */ export abstract class CollectionViewBase< - T, - SourceCollectionT extends ICollectionAndView -> extends SignalObserver implements IReadonlyCollection { + T, + SourceCollectionT extends ICollectionAndView + > + extends SignalObserver + implements IReadonlyCollection +{ private readonly _source: SourceCollectionT; private readonly _option: ICollectionViewOption; private _cachedItems: T[] = []; @@ -66,13 +73,35 @@ export abstract class CollectionViewBase< return this.filter(item) ? item : undefined; } - protected source_onChange(): void { + protected source_onChange(signal: CollectionChangeSignal): void { this.rebuildCache(); - this.notifyChange(); + this.notifyChange(signal); } - public notifyChange(): void { - this.notifyObservers(new CollectionChangeSignal(this)); + public notifyChange(signal: CollectionChangeSignal): void { + const changes = filterCollectionChangeDetail(signal.detail, this.filter); + + const addedCount = changes.added.length; + const removedCount = changes.removed.length; + const updatedCount = changes.updated.length; + + if (addedCount === 0 && removedCount === 0 && updatedCount === 0) { + return; + } + + this.notifyObservers(new CollectionChangeSignal(this, changes)); + + if (addedCount > 0) { + this.notifyObservers(new CollectionAddSignal(this, changes.added)); + } + + if (removedCount > 0) { + this.notifyObservers(new CollectionRemoveSignal(this, changes.removed)); + } + + if (updatedCount > 0) { + this.notifyObservers(new CollectionUpdateSignal(this, changes.updated)); + } } get count(): number { diff --git a/src/collections/IndexedCollectionBase.ts b/src/collections/IndexedCollectionBase.ts index 443886b..6359fa6 100644 --- a/src/collections/IndexedCollectionBase.ts +++ b/src/collections/IndexedCollectionBase.ts @@ -1,4 +1,5 @@ import { CollectionNature } from '../core/CollectionNature'; +import { ICollectionChangeDetail } from '../core/ICollectionChangeDetail'; import { ICollectionOption } from '../core/ICollectionOption'; import { IIndex } from '../core/IIndex'; import { IMutableCollection } from '../core/IMutableCollection'; @@ -7,8 +8,12 @@ import { defaultCollectionOption } from '../core/defaultCollectionOption'; import { IInternalList } from '../core/internals/IInternalList'; import { InternalList } from '../core/internals/InternalList'; import { InternalSetList } from '../core/internals/InternalSetList'; +import { CollectionAddSignal } from '../signals/CollectionAddSignal'; import { CollectionChangeSignal } from '../signals/CollectionChangeSignal'; +import { CollectionRemoveSignal } from '../signals/CollectionRemoveSignal'; +import { CollectionUpdateSignal } from '../signals/CollectionUpdateSignal'; import { SignalObserver } from '../signals/SignalObserver'; +import { mergeCollectionChangeDetail } from './util'; export abstract class IndexedCollectionBase extends SignalObserver @@ -20,6 +25,7 @@ export abstract class IndexedCollectionBase private _pauseChangeSignal: boolean = false; private _hasPendingChangeSignal: boolean = false; + private _pendingChange: Partial> = {}; public readonly option: Readonly; @@ -83,17 +89,15 @@ export abstract class IndexedCollectionBase for (const index of this.indexes) { index.index(item); } - this.notifyChange(); + this.notifyChange({ + added: [item], + }); return true; } public addRange( items: readonly T[] | IReadonlyCollection | ReadonlySet ): boolean[] { - // const rawItems: readonly T[] = Array.isArray(items) - // ? items - // : (items as IReadonlyCollection).items; - let rawItems: Readonly>; if (Array.isArray(items)) { rawItems = items; @@ -131,7 +135,9 @@ export abstract class IndexedCollectionBase for (const index of this.indexes) { index.unIndex(item); } - this.notifyChange(); + this.notifyChange({ + removed: [item], + }); return true; } @@ -149,7 +155,9 @@ export abstract class IndexedCollectionBase for (const index of this.indexes) { index.index(newItem); } - this.notifyChange(); + this.notifyChange({ + updated: [{ oldValue: oldItem, newValue: newItem }], + }); return true; } @@ -161,12 +169,30 @@ export abstract class IndexedCollectionBase return this._allItemList.count; } - protected notifyChange(): void { + protected notifyChange(change: Partial>): void { if (this._pauseChangeSignal) { this._hasPendingChangeSignal = true; + this._pendingChange = mergeCollectionChangeDetail( + this._pendingChange, + change + ); return; } - this.notifyObservers(new CollectionChangeSignal(this)); + + const changes = mergeCollectionChangeDetail({}, change); + this.notifyObservers(new CollectionChangeSignal(this, changes)); + + if (changes.added.length > 0) { + this.notifyObservers(new CollectionAddSignal(this, changes.added)); + } + + if (changes.removed.length > 0) { + this.notifyObservers(new CollectionRemoveSignal(this, changes.removed)); + } + + if (changes.updated.length > 0) { + this.notifyObservers(new CollectionUpdateSignal(this, changes.updated)); + } } /** @@ -194,7 +220,8 @@ export abstract class IndexedCollectionBase this._pauseChangeSignal = false; if (this._hasPendingChangeSignal) { this._hasPendingChangeSignal = false; - this.notifyChange(); + this.notifyChange(this._pendingChange); + this._pendingChange = {}; } } } diff --git a/src/collections/PrimaryKeyCollection.ts b/src/collections/PrimaryKeyCollection.ts index 3c6618b..8d02aaa 100644 --- a/src/collections/PrimaryKeyCollection.ts +++ b/src/collections/PrimaryKeyCollection.ts @@ -28,6 +28,21 @@ export class PrimaryKeyCollection< } } + protected override buildIndexes( + indexes: readonly IIndex[], + autoReindex?: boolean + ): void { + const combinedIndex: IIndex[] = []; + if (this.idIndex != null) { + // this.idIndex can be null during instantiation + combinedIndex.push(this.idIndex); + } + if (indexes != null && indexes.length > 0) { + combinedIndex.push(...indexes); + } + super.buildIndexes(combinedIndex, autoReindex); + } + exists(item: T): boolean { const key = this.primaryKeyExtract(item); return Boolean(this.byPrimaryKey(key)); diff --git a/src/collections/util.ts b/src/collections/util.ts new file mode 100644 index 0000000..7089d6f --- /dev/null +++ b/src/collections/util.ts @@ -0,0 +1,41 @@ +import { ICollectionChangeDetail } from '../core/ICollectionChangeDetail'; + +export function mergeCollectionChangeDetail( + a: Partial>, + b: Partial> +): ICollectionChangeDetail { + const added = a.added + ? b.added + ? a.added.concat(b.added) + : a.added + : b.added; + const removed = a.removed + ? b.removed + ? a.removed.concat(b.removed) + : a.removed + : b.removed; + const updated = a.updated + ? b.updated + ? a.updated.concat(b.updated) + : a.updated + : b.updated; + return { + added: added ?? [], + removed: removed ?? [], + updated: updated ?? [], + }; +} + +export function filterCollectionChangeDetail( + change: Readonly>, + filter: (item: T) => boolean +): ICollectionChangeDetail { + const added = change.added.filter(filter); + const removed = change.removed.filter(filter); + const updated = change.updated.filter((item) => filter(item.newValue)); + return { + added, + removed, + updated, + }; +} diff --git a/src/core/ICollectionChangeDetail.ts b/src/core/ICollectionChangeDetail.ts new file mode 100644 index 0000000..7845431 --- /dev/null +++ b/src/core/ICollectionChangeDetail.ts @@ -0,0 +1,10 @@ +export interface ICollectionUpdateLineItem { + oldValue: T; + newValue: T; +} + +export interface ICollectionChangeDetail { + readonly added: T[]; + readonly removed: T[]; + readonly updated: ICollectionUpdateLineItem[]; +} diff --git a/src/signals/CollectionAddSignal.ts b/src/signals/CollectionAddSignal.ts new file mode 100644 index 0000000..07a73c2 --- /dev/null +++ b/src/signals/CollectionAddSignal.ts @@ -0,0 +1,12 @@ +import { IReadonlyCollection } from '../core/IReadonlyCollection'; +import { Signal } from './Signal'; + +export class CollectionAddSignal extends Signal { + static readonly type = Symbol('COLLECTION_ADD'); + constructor( + target: IReadonlyCollection, + public readonly added: readonly T[] + ) { + super(CollectionAddSignal.type, target); + } +} diff --git a/src/signals/CollectionChangeSignal.ts b/src/signals/CollectionChangeSignal.ts index 1605182..643d6ac 100644 --- a/src/signals/CollectionChangeSignal.ts +++ b/src/signals/CollectionChangeSignal.ts @@ -1,9 +1,13 @@ -import { Signal } from './Signal'; +import { ICollectionChangeDetail } from '../core/ICollectionChangeDetail'; import { IReadonlyCollection } from '../core/IReadonlyCollection'; +import { Signal } from './Signal'; export class CollectionChangeSignal extends Signal { static readonly type = Symbol('COLLECTION_CHANGE'); - constructor(target: IReadonlyCollection) { + constructor( + target: IReadonlyCollection, + public readonly detail: Readonly> + ) { super(CollectionChangeSignal.type, target); } } diff --git a/src/signals/CollectionRemoveSignal.ts b/src/signals/CollectionRemoveSignal.ts new file mode 100644 index 0000000..110ac2a --- /dev/null +++ b/src/signals/CollectionRemoveSignal.ts @@ -0,0 +1,12 @@ +import { IReadonlyCollection } from '../core/IReadonlyCollection'; +import { Signal } from './Signal'; + +export class CollectionRemoveSignal extends Signal { + static readonly type = Symbol('COLLECTION_REMOVE'); + constructor( + target: IReadonlyCollection, + public readonly removed: readonly T[] + ) { + super(CollectionRemoveSignal.type, target); + } +} diff --git a/src/signals/CollectionUpdateSignal.ts b/src/signals/CollectionUpdateSignal.ts new file mode 100644 index 0000000..1719af2 --- /dev/null +++ b/src/signals/CollectionUpdateSignal.ts @@ -0,0 +1,13 @@ +import { ICollectionUpdateLineItem } from '../core/ICollectionChangeDetail'; +import { IReadonlyCollection } from '../core/IReadonlyCollection'; +import { Signal } from './Signal'; + +export class CollectionUpdateSignal extends Signal { + static readonly type = Symbol('COLLECTION_UPDATE'); + constructor( + target: IReadonlyCollection, + public readonly updated: readonly Readonly>[] + ) { + super(CollectionUpdateSignal.type, target); + } +} diff --git a/src/signals/index.ts b/src/signals/index.ts index 33d1e0c..2f3f414 100644 --- a/src/signals/index.ts +++ b/src/signals/index.ts @@ -1,4 +1,7 @@ +export { CollectionAddSignal } from './CollectionAddSignal'; export { CollectionChangeSignal } from './CollectionChangeSignal'; +export { CollectionRemoveSignal } from './CollectionRemoveSignal'; +export { CollectionUpdateSignal } from './CollectionUpdateSignal'; export { SignalHandler } from './ISignalObserver'; export { Signal, SignalType } from './Signal'; export { SignalObserver } from './SignalObserver'; diff --git a/test/CollectionSignal.test.ts b/test/CollectionSignal.test.ts new file mode 100644 index 0000000..9f996f1 --- /dev/null +++ b/test/CollectionSignal.test.ts @@ -0,0 +1,138 @@ +import { CollectionAddSignal } from '../src/signals/CollectionAddSignal'; +import { CollectionChangeSignal } from '../src/signals/CollectionChangeSignal'; +import { CollectionRemoveSignal } from '../src/signals/CollectionRemoveSignal'; +import { CollectionUpdateSignal } from '../src/signals/CollectionUpdateSignal'; +import { CarCollection, UsedCarCollectionView } from './shared/collections'; +import { + ICar, + newTeslaModel3, + newTeslaModelX, + usedChevyCamero, + usedTeslaModel3, +} from './shared/data'; + +describe('Collection signal tests', () => { + let changeSignal: jest.Mock>; + let addSignal: jest.Mock>; + let removeSignal: jest.Mock>; + let updateSignal: jest.Mock>; + + let changeViewSignal: jest.Mock>; + let addViewSignal: jest.Mock>; + let removeViewSignal: jest.Mock>; + let updateViewSignal: jest.Mock>; + + let carCollection: CarCollection; + let usedCarCollection: UsedCarCollectionView; + + beforeEach(() => { + changeSignal = jest.fn(); + addSignal = jest.fn(); + removeSignal = jest.fn(); + updateSignal = jest.fn(); + + changeViewSignal = jest.fn(); + addViewSignal = jest.fn(); + removeViewSignal = jest.fn(); + updateViewSignal = jest.fn(); + + carCollection = new CarCollection([newTeslaModel3]); + carCollection.registerObserver(CollectionChangeSignal.type, changeSignal); + carCollection.registerObserver(CollectionAddSignal.type, addSignal); + carCollection.registerObserver(CollectionRemoveSignal.type, removeSignal); + carCollection.registerObserver(CollectionUpdateSignal.type, updateSignal); + + usedCarCollection = new UsedCarCollectionView(carCollection); + usedCarCollection.registerObserver( + CollectionChangeSignal.type, + changeViewSignal + ); + usedCarCollection.registerObserver(CollectionAddSignal.type, addViewSignal); + usedCarCollection.registerObserver( + CollectionRemoveSignal.type, + removeViewSignal + ); + usedCarCollection.registerObserver( + CollectionUpdateSignal.type, + updateViewSignal + ); + }); + + describe('When adding a new car to the collection', () => { + beforeEach(() => { + carCollection.add(newTeslaModelX); + }); + + test('Should trigger change signal in collection', () => { + expect(changeSignal).toHaveBeenCalledTimes(1); + }); + + test('Should trigger add signal in collection', () => { + expect(addSignal).toHaveBeenCalledTimes(1); + }); + + test('Should trigger no change signal in view', () => { + expect(changeViewSignal).toHaveBeenCalledTimes(0); + }); + + test('Should trigger no add signal in view', () => { + expect(addViewSignal).toHaveBeenCalledTimes(0); + }); + + test('Should trigger no remove signal in view', () => { + expect(removeViewSignal).toHaveBeenCalledTimes(0); + }); + + test('Should trigger no update signal in view', () => { + expect(updateViewSignal).toHaveBeenCalledTimes(0); + }); + }); + + describe('When adding a used car to the collection', () => { + beforeEach(() => { + carCollection.add(usedChevyCamero); + }); + + test('Should trigger change signal in collection', () => { + expect(changeSignal).toHaveBeenCalledTimes(1); + }); + + test('Should trigger add signal in collection', () => { + expect(addSignal).toHaveBeenCalledTimes(1); + }); + + test('Should trigger change signal in view', () => { + expect(changeViewSignal).toHaveBeenCalledTimes(1); + }); + + test('Should trigger add signal in view', () => { + expect(addViewSignal).toHaveBeenCalledTimes(1); + }); + + test('Should trigger no remove signal in view', () => { + expect(removeViewSignal).toHaveBeenCalledTimes(0); + }); + + test('Should trigger no update signal in view', () => { + expect(updateViewSignal).toHaveBeenCalledTimes(0); + }); + + describe('When unregister add signal', () => { + beforeEach(() => { + changeViewSignal.mockClear(); + changeSignal.mockClear(); + carCollection.unregisterObserver(changeSignal); + usedCarCollection.unregisterObserver(changeViewSignal); + carCollection.add(usedTeslaModel3); + }); + + test("should not trigger collection's change signal", () => { + expect(changeSignal).toHaveBeenCalledTimes(0); + }); + + test("should not trigger view's change signal", () => { + expect(changeViewSignal).toHaveBeenCalledTimes(0); + }); + }); + }); +}); diff --git a/test/collectionview.test.ts b/test/collectionview.test.ts index febf143..7daf691 100644 --- a/test/collectionview.test.ts +++ b/test/collectionview.test.ts @@ -1,46 +1,24 @@ import { CollectionViewBase } from '../src'; import { - allCars, - AttributeTag, + CarCollection, + UsedCarCollectionView, + UsedGasCarCollectionView, +} from './shared/collections'; +import { ICar, + allCars, usedChevyCamero, usedChevyEquinox, usedTeslaModel3, usedTeslaModelX, } from './shared/data'; -import { CarCollection } from './shared/collections'; - -class UsedCarCollectionView extends CollectionViewBase { - constructor(source: CarCollection) { - super(source, { - filter: car => !car.isNew, - }); - } - - byMake(make: string): readonly ICar[] { - return super.applyFilterAndSort(this.source.byMake(make)); - } - - byIsNew(isNew: boolean): readonly ICar[] { - return super.applyFilterAndSort(this.source.byIsNew(isNew)); - } -} /** - * Nested view that's based on UsedCarCollection + * View with out any filter or sort set */ -class UsedGasCarCollectionView extends CollectionViewBase< - ICar, - UsedCarCollectionView -> { - constructor(source: UsedCarCollectionView) { - super(source, { - filter: car => car.tags.includes(AttributeTag.Gas), - }); - } - - byMake(make: string): readonly ICar[] { - return super.applyFilterAndSort(this.source.byMake(make)); +class DefaultView extends CollectionViewBase { + constructor(source: CarCollection) { + super(source); } } @@ -48,15 +26,23 @@ describe('collection view tests', () => { let cars: CarCollection; let usedCars: UsedCarCollectionView; let usedGasCars: UsedGasCarCollectionView; + let defaultView: DefaultView; beforeEach(() => { cars = new CarCollection(); cars.addRange(allCars); + defaultView = new DefaultView(cars); usedCars = new UsedCarCollectionView(cars); usedGasCars = new UsedGasCarCollectionView(usedCars); }); + describe('DefaultView', () => { + it('defaultView has same number of items as collection', () => { + expect(defaultView.count).toEqual(cars.count); + }); + }); + // Tests against index which is only based on one value describe('OneLevelIndex', () => { it('usedCars.byMake(Tesla) should return all used Tesla cars', () => { @@ -102,5 +88,13 @@ describe('collection view tests', () => { new Set([usedTeslaModelX]) ); }); + + it('exist should return false with the removed car', () => { + expect(usedCars.exists(usedTeslaModel3)).toEqual(false); + }); + + it('exist should return true with cars not removed', () => { + expect(usedCars.exists(usedTeslaModelX)).toEqual(true); + }); }); }); diff --git a/test/shared/collections.ts b/test/shared/collections.ts index 0cde579..409015e 100644 --- a/test/shared/collections.ts +++ b/test/shared/collections.ts @@ -1,5 +1,6 @@ import { CollectionIndex, + CollectionViewBase, ICollectionOption, IndexedCollectionBase, } from '../../src'; @@ -59,3 +60,40 @@ export class CarCollection extends IndexedCollectionBase { return this.byTagByPriceRangeIndex.getValue(tag, priceRange); } } + +export class UsedCarCollectionView extends CollectionViewBase< + ICar, + CarCollection +> { + constructor(source: CarCollection) { + super(source, { + filter: (car) => !car.isNew, + }); + } + + byMake(make: string): readonly ICar[] { + return super.applyFilterAndSort(this.source.byMake(make)); + } + + byIsNew(isNew: boolean): readonly ICar[] { + return super.applyFilterAndSort(this.source.byIsNew(isNew)); + } +} + +/** + * Nested view that's based on UsedCarCollection + */ +export class UsedGasCarCollectionView extends CollectionViewBase< + ICar, + UsedCarCollectionView +> { + constructor(source: UsedCarCollectionView) { + super(source, { + filter: (car) => car.tags.includes(AttributeTag.Gas), + }); + } + + byMake(make: string): readonly ICar[] { + return super.applyFilterAndSort(this.source.byMake(make)); + } +}