From 37613976fb3a1569a439ce6926013164d3854a4e Mon Sep 17 00:00:00 2001 From: Tore Hammervoll Date: Mon, 11 Mar 2019 16:31:15 +0100 Subject: [PATCH 1/6] Merge changes to resource instances lodash merge only recursively merges arrays and plain objects, not class instances. Use lodash mergeWith and a customizer that can handle instances of Resource. The `resourceCustomizer` function will merge Resources recursively --- src/state/reducer.ts | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/state/reducer.ts b/src/state/reducer.ts index caeb5a420cb4..5600973799bb 100644 --- a/src/state/reducer.ts +++ b/src/state/reducer.ts @@ -1,5 +1,5 @@ import { normalize } from '../resource'; -import { merge } from 'lodash'; +import { mergeWith } from 'lodash'; import { Resource } from '../resource'; import { ActionTypes, State } from '../types'; @@ -13,6 +13,14 @@ type Writable = { [P in keyof T]: NonNullable; } +function resourceCustomizer(a, b) { + if (b instanceof Resource) { + const merged = mergeWith({ ...a }, b, resourceCustomizer); + + return Object.assign(b, merged); + } +} + export default function reducer(state: State, action: ActionTypes) { switch (action.type) { case 'receive': @@ -31,7 +39,7 @@ export default function reducer(state: State, action: ActionTypes) { } const normalized = normalize(action.payload, action.meta.schema); return { - entities: merge({ ...state.entities }, normalized.entities), + entities: mergeWith({ ...state.entities }, normalized.entities, resourceCustomizer), results: { ...state.results, [action.meta.url]: normalized.result, @@ -49,7 +57,7 @@ export default function reducer(state: State, action: ActionTypes) { let { entities } = normalize(action.payload, action.meta.schema); return { ...state, - entities: merge({ ...state.entities }, entities), + entities: mergeWith({ ...state.entities }, entities, resourceCustomizer), }; case 'purge': if (action.error) return state; From 74345848d6d260f7a7e86e7806527464dcb9c48b Mon Sep 17 00:00:00 2001 From: Tore Hammervoll Date: Mon, 11 Mar 2019 16:56:19 +0100 Subject: [PATCH 2/6] Explicit any type Fixed indentation --- src/state/reducer.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/state/reducer.ts b/src/state/reducer.ts index 5600973799bb..94fbd3b8360a 100644 --- a/src/state/reducer.ts +++ b/src/state/reducer.ts @@ -13,12 +13,11 @@ type Writable = { [P in keyof T]: NonNullable; } -function resourceCustomizer(a, b) { - if (b instanceof Resource) { - const merged = mergeWith({ ...a }, b, resourceCustomizer); - - return Object.assign(b, merged); - } +function resourceCustomizer(a: any, b: any): any { + if (b instanceof Resource) { + const merged = mergeWith({ ...a }, b, resourceCustomizer); + return Object.assign(b, merged); + } } export default function reducer(state: State, action: ActionTypes) { From 8f309b6fd09876982921e87ac02aa36207b8e567 Mon Sep 17 00:00:00 2001 From: Tore Hammervoll Date: Wed, 27 Mar 2019 20:32:30 +0100 Subject: [PATCH 3/6] Added test case for entity merging Update test --- src/state/__tests__/reducer.ts | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/state/__tests__/reducer.ts b/src/state/__tests__/reducer.ts index 1144ec6ef768..af3e9f54c32b 100644 --- a/src/state/__tests__/reducer.ts +++ b/src/state/__tests__/reducer.ts @@ -15,6 +15,10 @@ describe('reducer', () => { expiresAt: 5000500000, }, }; + const partialResultAction: ReceiveAction = { + ...action, + payload: { id, title: 'hello' }, + }; const iniState = { entities: {}, results: {}, @@ -33,6 +37,21 @@ describe('reducer', () => { expect(nextEntity).not.toBe(prevEntity); expect(nextEntity).toBeDefined(); }) + it('should merge partial entity with existing entity', () => { + const getEntity = (state: any): ArticleResource => state.entities[ArticleResource.getKey()][`${ArticleResource.pk(action.payload)}`] + const prevEntity = getEntity(newState); + expect(prevEntity).toBeDefined(); + const nextState = reducer(newState, partialResultAction); + const nextEntity = getEntity(nextState); + expect(nextEntity).not.toBe(prevEntity); + expect(nextEntity).toBeDefined(); + + expect(nextEntity.title).not.toBe(prevEntity.title); + expect(nextEntity.title).toBe(partialResultAction.payload.title); + + expect(nextEntity.content).toBe(prevEntity.content); + expect(nextEntity.content).not.toBe(partialResultAction.payload.content); + }) }); it('mutate should never change results', () => { const id = 20; From 95b5361d5c6f530d8938b1964e667853922101eb Mon Sep 17 00:00:00 2001 From: Tore Hammervoll Date: Sat, 16 Mar 2019 19:26:06 +0100 Subject: [PATCH 4/6] use Resource.merge() to merge entities. --- src/state/reducer.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/state/reducer.ts b/src/state/reducer.ts index 94fbd3b8360a..2dd732fc864b 100644 --- a/src/state/reducer.ts +++ b/src/state/reducer.ts @@ -14,9 +14,9 @@ type Writable = { } function resourceCustomizer(a: any, b: any): any { - if (b instanceof Resource) { - const merged = mergeWith({ ...a }, b, resourceCustomizer); - return Object.assign(b, merged); + if (a instanceof Resource && b instanceof Resource) { + const Static = b.constructor as typeof Resource; + return Static.merge(a, b); } } From 28b7b2e685bb3ac17be45abcef719396eb5c97aa Mon Sep 17 00:00:00 2001 From: Tore Hammervoll Date: Thu, 28 Mar 2019 11:10:40 +0100 Subject: [PATCH 5/6] Clarify resourceCustomizer and add tests --- src/state/__tests__/reducer.ts | 61 +++++++++++++++++++++++++++++++++- src/state/reducer.ts | 7 ++-- 2 files changed, 65 insertions(+), 3 deletions(-) diff --git a/src/state/__tests__/reducer.ts b/src/state/__tests__/reducer.ts index af3e9f54c32b..ba107cf15029 100644 --- a/src/state/__tests__/reducer.ts +++ b/src/state/__tests__/reducer.ts @@ -1,6 +1,65 @@ import { ArticleResource, PaginatedArticleResource } from '../../__tests__/common'; -import reducer from '../reducer'; +import reducer, { resourceCustomizer } from '../reducer'; import { FetchAction, RPCAction, ReceiveAction, PurgeAction, State } from '../../types'; +import { mergeWith } from 'lodash'; + +describe('resourceCustomizer', () => { + it('should merge two Resource instances', () => { + const id = 20; + const a = ArticleResource.fromJS({ id, title: 'hi', content: 'this is the content' }); + const b = ArticleResource.fromJS({ id, title: 'hello' }); + + const merged = resourceCustomizer(a, b, "", undefined, undefined); + expect(merged).toBeInstanceOf(ArticleResource); + expect(merged).toEqual( + ArticleResource.fromJS({ + id, + title: 'hello', + content: 'this is the content' + }) + ) + }); + it('should handle merging of Resource instances when used with lodash.mergeWith()', () => { + const id = 20; + const entitiesA = { + [ArticleResource.getKey()]: { + [id]: ArticleResource.fromJS({ id, title: 'hi', content: 'this is the content' }), + }, + } + const entitiesB = { + [ArticleResource.getKey()]: { + [id]: ArticleResource.fromJS({ id, title: 'hello' }), + }, + } + + const merged = mergeWith({ ...entitiesA }, entitiesB, resourceCustomizer); + expect(merged[ArticleResource.getKey()][id]).toBeInstanceOf(ArticleResource); + expect(merged[ArticleResource.getKey()][id]).toEqual( + ArticleResource.fromJS({ + id, + title: 'hello', + content: 'this is the content' + }) + ) + }); + it('should not affect merging of plain objects when used with lodash.mergeWith()', () => { + const id = 20; + const entitiesA = { + [ArticleResource.getKey()]: { + [id]: ArticleResource.fromJS({ id, title: 'hi', content: 'this is the content' }), + [42]: ArticleResource.fromJS({ id: 42, title: 'dont touch me', content: 'this is mine' }), + }, + } + const entitiesB = { + [ArticleResource.getKey()]: { + [id]: ArticleResource.fromJS({ id, title: 'hi', content: 'this is the content' }), + }, + } + + const merged = mergeWith({ ...entitiesA }, entitiesB, resourceCustomizer); + expect(merged[ArticleResource.getKey()][42]).toBe(entitiesA[ArticleResource.getKey()][42]); + }); +}); describe('reducer', () => { describe('singles', () => { diff --git a/src/state/reducer.ts b/src/state/reducer.ts index 2dd732fc864b..952c024efacc 100644 --- a/src/state/reducer.ts +++ b/src/state/reducer.ts @@ -1,5 +1,5 @@ import { normalize } from '../resource'; -import { mergeWith } from 'lodash'; +import { mergeWith, MergeWithCustomizer } from 'lodash'; import { Resource } from '../resource'; import { ActionTypes, State } from '../types'; @@ -13,11 +13,14 @@ type Writable = { [P in keyof T]: NonNullable; } -function resourceCustomizer(a: any, b: any): any { +export const resourceCustomizer: MergeWithCustomizer = (a, b) => { if (a instanceof Resource && b instanceof Resource) { const Static = b.constructor as typeof Resource; return Static.merge(a, b); } + + // use default merging in lodash.merge() + return undefined; } export default function reducer(state: State, action: ActionTypes) { From 45d65998b0be971df11a1f77887d5ac744b35151 Mon Sep 17 00:00:00 2001 From: Tore Hammervoll Date: Fri, 29 Mar 2019 11:44:51 +0100 Subject: [PATCH 6/6] Check for merge() function on constructor. Remove type import from lodash --- src/state/__tests__/reducer.ts | 2 +- src/state/reducer.ts | 25 +++++++++++++++++++------ 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/state/__tests__/reducer.ts b/src/state/__tests__/reducer.ts index ba107cf15029..09e20b8b52bb 100644 --- a/src/state/__tests__/reducer.ts +++ b/src/state/__tests__/reducer.ts @@ -9,7 +9,7 @@ describe('resourceCustomizer', () => { const a = ArticleResource.fromJS({ id, title: 'hi', content: 'this is the content' }); const b = ArticleResource.fromJS({ id, title: 'hello' }); - const merged = resourceCustomizer(a, b, "", undefined, undefined); + const merged = resourceCustomizer(a, b); expect(merged).toBeInstanceOf(ArticleResource); expect(merged).toEqual( ArticleResource.fromJS({ diff --git a/src/state/reducer.ts b/src/state/reducer.ts index 952c024efacc..babe29a65dbd 100644 --- a/src/state/reducer.ts +++ b/src/state/reducer.ts @@ -1,5 +1,5 @@ import { normalize } from '../resource'; -import { mergeWith, MergeWithCustomizer } from 'lodash'; +import { mergeWith } from 'lodash'; import { Resource } from '../resource'; import { ActionTypes, State } from '../types'; @@ -13,15 +13,28 @@ type Writable = { [P in keyof T]: NonNullable; } -export const resourceCustomizer: MergeWithCustomizer = (a, b) => { - if (a instanceof Resource && b instanceof Resource) { - const Static = b.constructor as typeof Resource; +interface MergeableStatic { + new(): T; + merge(a: T, b: T): T; +} + +function isMergeable( + constructor: any +): constructor is MergeableStatic { + return ( + constructor && + typeof constructor.merge === 'function' + ); +} + +export const resourceCustomizer = (a: any, b: any): any => { + const Static = b && b.constructor; + if (a && Static && isMergeable(Static)) { return Static.merge(a, b); } // use default merging in lodash.merge() - return undefined; -} +}; export default function reducer(state: State, action: ActionTypes) { switch (action.type) {