From 36442582529c2ad91d5f32353f096c1c45665b2b Mon Sep 17 00:00:00 2001 From: Bruno Baia Date: Sun, 19 Nov 2017 00:23:28 +0100 Subject: [PATCH] fix(Entity): updateOne/updateMany should not change ids state on existing entity Closes #571 --- .../spec/unsorted_state_adapter.spec.ts | 15 ++++ modules/entity/src/sorted_state_adapter.ts | 32 ++++----- modules/entity/src/state_adapter.ts | 19 ++++- modules/entity/src/unsorted_state_adapter.ts | 71 +++++++++++-------- 4 files changed, 88 insertions(+), 49 deletions(-) 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..f663d14734 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,13 +20,13 @@ 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) ); @@ -34,18 +34,18 @@ export function createSortedStateAdapter(selectId: any, sort: any): any { return merge(models, state); } - 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); } @@ -63,8 +63,8 @@ export function createSortedStateAdapter(selectId: any, sort: any): any { models.push(updated); } - 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)); @@ -76,10 +76,10 @@ export function createSortedStateAdapter(selectId: any, sort: any): any { return merge(models, state); } - function merge(models: T[], state: R): boolean; - function merge(models: any[], state: any): boolean { + function merge(models: T[], state: R): DidMutate; + function merge(models: any[], state: any): DidMutate { if (models.length === 0) { - return false; + return DidMutate.None; } models.sort(sort); @@ -114,7 +114,7 @@ export function createSortedStateAdapter(selectId: any, sort: any): any { state.entities[selectId(model)] = model; }); - return true; + return DidMutate.Both; } 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 {