From b989e4b00e4607fef3b9a97d5243a539488f42fa Mon Sep 17 00:00:00 2001 From: Bruno Baia Date: Tue, 28 Nov 2017 03:16:14 +0100 Subject: [PATCH] fix(Entity): updateOne/updateMany should not change ids state on existing entity (#581) Closes #571 --- .../entity/spec/sorted_state_adapter.spec.ts | 46 +++++++++++ .../spec/unsorted_state_adapter.spec.ts | 15 ++++ modules/entity/src/sorted_state_adapter.ts | 82 ++++++++++++------- modules/entity/src/state_adapter.ts | 19 ++++- modules/entity/src/unsorted_state_adapter.ts | 71 +++++++++------- 5 files changed, 171 insertions(+), 62 deletions(-) diff --git a/modules/entity/spec/sorted_state_adapter.spec.ts b/modules/entity/spec/sorted_state_adapter.spec.ts index ca4fe2ef48..e88c12adc4 100644 --- a/modules/entity/spec/sorted_state_adapter.spec.ts +++ b/modules/entity/spec/sorted_state_adapter.spec.ts @@ -153,6 +153,24 @@ describe('Sorted State Adapter', () => { expect(withUpdates).toBe(state); }); + it('should not change ids state if you attempt to update an entity that does not impact sorting', () => { + const withAll = adapter.addAll( + [TheGreatGatsby, AClockworkOrange, AnimalFarm], + state + ); + const changes = { title: 'The Great Gatsby II' }; + + const withUpdates = adapter.updateOne( + { + id: TheGreatGatsby.id, + changes, + }, + withAll + ); + + expect(withAll.ids).toBe(withUpdates.ids); + }); + it('should let you update the id of entity', () => { const withOne = adapter.addOne(TheGreatGatsby, state); const changes = { id: 'A New Id' }; @@ -176,6 +194,34 @@ describe('Sorted State Adapter', () => { }); }); + it('should resort correctly if same id but sort key update', () => { + const withAll = adapter.addAll( + [TheGreatGatsby, AnimalFarm, AClockworkOrange], + state + ); + const changes = { title: 'A New Hope' }; + + const withUpdates = adapter.updateOne( + { + id: TheGreatGatsby.id, + changes, + }, + withAll + ); + + expect(withUpdates).toEqual({ + ids: [AClockworkOrange.id, TheGreatGatsby.id, AnimalFarm.id], + entities: { + [AClockworkOrange.id]: AClockworkOrange, + [TheGreatGatsby.id]: { + ...TheGreatGatsby, + ...changes, + }, + [AnimalFarm.id]: AnimalFarm, + }, + }); + }); + it('should resort correctly if the id and sort key update', () => { const withOne = adapter.addAll( [TheGreatGatsby, AnimalFarm, AClockworkOrange], diff --git a/modules/entity/spec/unsorted_state_adapter.spec.ts b/modules/entity/spec/unsorted_state_adapter.spec.ts index 941ab87088..0d94a28b2b 100644 --- a/modules/entity/spec/unsorted_state_adapter.spec.ts +++ b/modules/entity/spec/unsorted_state_adapter.spec.ts @@ -152,6 +152,21 @@ describe('Unsorted State Adapter', () => { expect(withUpdates).toBe(state); }); + it('should not change ids state if you attempt to update an entity that has already been added', () => { + const withOne = adapter.addOne(TheGreatGatsby, state); + const changes = { title: 'A New Hope' }; + + const withUpdates = adapter.updateOne( + { + id: TheGreatGatsby.id, + changes, + }, + withOne + ); + + expect(withOne.ids).toBe(withUpdates.ids); + }); + it('should let you update the id of entity', () => { const withOne = adapter.addOne(TheGreatGatsby, state); const changes = { id: 'A New Id' }; diff --git a/modules/entity/src/sorted_state_adapter.ts b/modules/entity/src/sorted_state_adapter.ts index 4ae8e694b6..0641234b65 100644 --- a/modules/entity/src/sorted_state_adapter.ts +++ b/modules/entity/src/sorted_state_adapter.ts @@ -6,7 +6,7 @@ import { EntityStateAdapter, Update, } from './models'; -import { createStateOperator } from './state_adapter'; +import { createStateOperator, DidMutate } from './state_adapter'; import { createUnsortedStateAdapter } from './unsorted_state_adapter'; export function createSortedStateAdapter( @@ -20,68 +20,94 @@ export function createSortedStateAdapter(selectId: any, sort: any): any { selectId ); - function addOneMutably(entity: T, state: R): boolean; - function addOneMutably(entity: any, state: any): boolean { + function addOneMutably(entity: T, state: R): DidMutate; + function addOneMutably(entity: any, state: any): DidMutate { return addManyMutably([entity], state); } - function addManyMutably(newModels: T[], state: R): boolean; - function addManyMutably(newModels: any[], state: any): boolean { + function addManyMutably(newModels: T[], state: R): DidMutate; + function addManyMutably(newModels: any[], state: any): DidMutate { const models = newModels.filter( model => !(selectId(model) in state.entities) ); - return merge(models, state); + if (models.length === 0) { + return DidMutate.None; + } else { + merge(models, state); + return DidMutate.Both; + } } - function addAllMutably(models: T[], state: R): boolean; - function addAllMutably(models: any[], state: any): boolean { + function addAllMutably(models: T[], state: R): DidMutate; + function addAllMutably(models: any[], state: any): DidMutate { state.entities = {}; state.ids = []; addManyMutably(models, state); - return true; + return DidMutate.Both; } - function updateOneMutably(update: Update, state: R): boolean; - function updateOneMutably(update: any, state: any): boolean { + function updateOneMutably(update: Update, state: R): DidMutate; + function updateOneMutably(update: any, state: any): DidMutate { return updateManyMutably([update], state); } - function takeUpdatedModel(models: T[], update: Update, state: R): void; - function takeUpdatedModel(models: any[], update: any, state: any): void { + function takeUpdatedModel(models: T[], update: Update, state: R): boolean; + function takeUpdatedModel(models: any[], update: any, state: any): boolean { if (!(update.id in state.entities)) { - return; + return false; } const original = state.entities[update.id]; const updated = Object.assign({}, original, update.changes); + const newKey = selectId(updated); delete state.entities[update.id]; models.push(updated); + + return newKey !== update.id; } - function updateManyMutably(updates: Update[], state: R): boolean; - function updateManyMutably(updates: any[], state: any): boolean { + function updateManyMutably(updates: Update[], state: R): DidMutate; + function updateManyMutably(updates: any[], state: any): DidMutate { const models: T[] = []; - updates.forEach(update => takeUpdatedModel(models, update, state)); - - if (models.length) { - state.ids = state.ids.filter((id: any) => id in state.entities); - } + const didMutateIds = + updates.filter(update => takeUpdatedModel(models, update, state)).length > + 0; - return merge(models, state); - } - - function merge(models: T[], state: R): boolean; - function merge(models: any[], state: any): boolean { if (models.length === 0) { - return false; + return DidMutate.None; + } else { + const originalIds = state.ids; + const updatedIndexes: any[] = []; + state.ids = state.ids.filter((id: any, index: number) => { + if (id in state.entities) { + return true; + } else { + updatedIndexes.push(index); + return false; + } + }); + + merge(models, state); + + if ( + !didMutateIds && + updatedIndexes.every((i: number) => state.ids[i] === originalIds[i]) + ) { + return DidMutate.EntitiesOnly; + } else { + return DidMutate.Both; + } } + } + function merge(models: T[], state: R): void; + function merge(models: any[], state: any): void { models.sort(sort); const ids: any[] = []; @@ -113,8 +139,6 @@ export function createSortedStateAdapter(selectId: any, sort: any): any { models.forEach((model, i) => { state.entities[selectId(model)] = model; }); - - return true; } return { diff --git a/modules/entity/src/state_adapter.ts b/modules/entity/src/state_adapter.ts index bf1fd7da77..aafb6dc3ab 100644 --- a/modules/entity/src/state_adapter.ts +++ b/modules/entity/src/state_adapter.ts @@ -1,10 +1,16 @@ import { EntityState, EntityStateAdapter } from './models'; +export enum DidMutate { + EntitiesOnly, + Both, + None, +} + export function createStateOperator( - mutator: (arg: R, state: EntityState) => boolean + mutator: (arg: R, state: EntityState) => DidMutate ): EntityState; export function createStateOperator( - mutator: (arg: any, state: any) => boolean + mutator: (arg: any, state: any) => DidMutate ): any { return function operation>(arg: R, state: any): S { const clonedEntityState: EntityState = { @@ -14,10 +20,17 @@ export function createStateOperator( const didMutate = mutator(arg, clonedEntityState); - if (didMutate) { + if (didMutate === DidMutate.Both) { return Object.assign({}, state, clonedEntityState); } + if (didMutate === DidMutate.EntitiesOnly) { + return { + ...state, + entities: clonedEntityState.entities, + }; + } + return state; }; } diff --git a/modules/entity/src/unsorted_state_adapter.ts b/modules/entity/src/unsorted_state_adapter.ts index 37057137ab..dcf1d53214 100644 --- a/modules/entity/src/unsorted_state_adapter.ts +++ b/modules/entity/src/unsorted_state_adapter.ts @@ -1,5 +1,5 @@ import { EntityState, EntityStateAdapter, IdSelector, Update } from './models'; -import { createStateOperator } from './state_adapter'; +import { createStateOperator, DidMutate } from './state_adapter'; export function createUnsortedStateAdapter( selectId: IdSelector @@ -7,48 +7,49 @@ export function createUnsortedStateAdapter( export function createUnsortedStateAdapter(selectId: IdSelector): any { type R = EntityState; - function addOneMutably(entity: T, state: R): boolean; - function addOneMutably(entity: any, state: any): boolean { + function addOneMutably(entity: T, state: R): DidMutate; + function addOneMutably(entity: any, state: any): DidMutate { const key = selectId(entity); if (key in state.entities) { - return false; + return DidMutate.None; } state.ids.push(key); state.entities[key] = entity; - return true; + return DidMutate.Both; } - function addManyMutably(entities: T[], state: R): boolean; - function addManyMutably(entities: any[], state: any): boolean { + function addManyMutably(entities: T[], state: R): DidMutate; + function addManyMutably(entities: any[], state: any): DidMutate { let didMutate = false; for (let index in entities) { - didMutate = addOneMutably(entities[index], state) || didMutate; + didMutate = + addOneMutably(entities[index], state) !== DidMutate.None || didMutate; } - return didMutate; + return didMutate ? DidMutate.Both : DidMutate.None; } - function addAllMutably(entities: T[], state: R): boolean; - function addAllMutably(entities: any[], state: any): boolean { + function addAllMutably(entities: T[], state: R): DidMutate; + function addAllMutably(entities: any[], state: any): DidMutate { state.ids = []; state.entities = {}; addManyMutably(entities, state); - return true; + return DidMutate.Both; } - function removeOneMutably(key: T, state: R): boolean; - function removeOneMutably(key: any, state: any): boolean { + function removeOneMutably(key: T, state: R): DidMutate; + function removeOneMutably(key: any, state: any): DidMutate { return removeManyMutably([key], state); } - function removeManyMutably(keys: T[], state: R): boolean; - function removeManyMutably(keys: any[], state: any): boolean { + function removeManyMutably(keys: T[], state: R): DidMutate; + function removeManyMutably(keys: any[], state: any): DidMutate { const didMutate = keys .filter(key => key in state.entities) @@ -58,7 +59,7 @@ export function createUnsortedStateAdapter(selectId: IdSelector): any { state.ids = state.ids.filter((id: any) => id in state.entities); } - return didMutate; + return didMutate ? DidMutate.Both : DidMutate.None; } function removeAll(state: S): S; @@ -78,38 +79,48 @@ export function createUnsortedStateAdapter(selectId: IdSelector): any { keys: { [id: string]: any }, update: Update, state: any - ): void { + ): boolean { const original = state.entities[update.id]; const updated: T = Object.assign({}, original, update.changes); const newKey = selectId(updated); + const hasNewKey = newKey !== update.id; - if (newKey !== update.id) { + if (hasNewKey) { keys[update.id] = newKey; delete state.entities[update.id]; } state.entities[newKey] = updated; + + return hasNewKey; } - function updateOneMutably(update: Update, state: R): boolean; - function updateOneMutably(update: any, state: any): boolean { + function updateOneMutably(update: Update, state: R): DidMutate; + function updateOneMutably(update: any, state: any): DidMutate { return updateManyMutably([update], state); } - function updateManyMutably(updates: Update[], state: R): boolean; - function updateManyMutably(updates: any[], state: any): boolean { + function updateManyMutably(updates: Update[], state: R): DidMutate; + function updateManyMutably(updates: any[], state: any): DidMutate { const newKeys: { [id: string]: string } = {}; - const didMutate = - updates - .filter(update => update.id in state.entities) - .map(update => takeNewKey(newKeys, update, state)).length > 0; + updates = updates.filter(update => update.id in state.entities); - if (didMutate) { - state.ids = state.ids.map((id: any) => newKeys[id] || id); + const didMutateEntities = updates.length > 0; + + if (didMutateEntities) { + const didMutateIds = + updates.filter(update => takeNewKey(newKeys, update, state)).length > 0; + + if (didMutateIds) { + state.ids = state.ids.map((id: any) => newKeys[id] || id); + return DidMutate.Both; + } else { + return DidMutate.EntitiesOnly; + } } - return didMutate; + return DidMutate.None; } return {