From acc96c12946e92f9284941f6132140dd8cc70f7a Mon Sep 17 00:00:00 2001 From: noa cohen Date: Sun, 14 Jul 2024 16:01:03 +0300 Subject: [PATCH 1/6] wrapWithShouldUpdateExperimental --- src/connectWithShell.tsx | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/src/connectWithShell.tsx b/src/connectWithShell.tsx index cc0067f..106cdfc 100644 --- a/src/connectWithShell.tsx +++ b/src/connectWithShell.tsx @@ -39,6 +39,31 @@ function wrapWithShouldUpdate) => (shouldUpdate && !shouldUpdate(shell, getOwnProps()) ? true : func(args[0], args[1]))) as F } +function wrapWithShouldUpdateExperimental boolean>( + shouldUpdate: Maybe<(shell: Shell, ownProps?: Props) => boolean>, + arePropsEqual: F, + getOwnProps: () => Props, + shell: Shell +): F { + if (!shouldUpdate) { + return arePropsEqual + } + let changeInPropsDetected = false + return ((...args: Parameters) => { + if (!shouldUpdate(shell, getOwnProps())) { + if (!changeInPropsDetected) { + changeInPropsDetected = !arePropsEqual(args[0], args[1]) + } + return true + } + if (changeInPropsDetected) { + changeInPropsDetected = false + return false + } + return arePropsEqual(args[0], args[1]) + }) as F +} + function wrapWithShellContext( component: React.ComponentType, mapStateToProps: MapStateToProps, @@ -98,7 +123,7 @@ function wrapWithShellContext( this.getOwnProps, boundShell ), - areOwnPropsEqual: wrapWithShouldUpdate( + areOwnPropsEqual: wrapWithShouldUpdateExperimental( shouldComponentUpdate, reduxConnectOptions.areOwnPropsEqual, this.getOwnProps, From a6fb845fa79a92fcdf43dccae4ff0ed08d69cb79 Mon Sep 17 00:00:00 2001 From: noa cohen Date: Mon, 15 Jul 2024 12:12:32 +0300 Subject: [PATCH 2/6] improve code readability --- src/connectWithShell.tsx | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/src/connectWithShell.tsx b/src/connectWithShell.tsx index 106cdfc..1421fa4 100644 --- a/src/connectWithShell.tsx +++ b/src/connectWithShell.tsx @@ -39,28 +39,29 @@ function wrapWithShouldUpdate) => (shouldUpdate && !shouldUpdate(shell, getOwnProps()) ? true : func(args[0], args[1]))) as F } -function wrapWithShouldUpdateExperimental boolean>( - shouldUpdate: Maybe<(shell: Shell, ownProps?: Props) => boolean>, - arePropsEqual: F, +function arePropsEqualFuncWrapper boolean>( + componentShouldUpdateFunc: Maybe<(shell: Shell, ownProps?: Props) => boolean>, + arePropsEqualFunc: F, getOwnProps: () => Props, shell: Shell ): F { - if (!shouldUpdate) { - return arePropsEqual + if (!componentShouldUpdateFunc) { + return arePropsEqualFunc } let changeInPropsDetected = false return ((...args: Parameters) => { - if (!shouldUpdate(shell, getOwnProps())) { - if (!changeInPropsDetected) { - changeInPropsDetected = !arePropsEqual(args[0], args[1]) + const componentShouldUpdate = componentShouldUpdateFunc(shell, getOwnProps()) + if (componentShouldUpdate) { + if (changeInPropsDetected) { + changeInPropsDetected = false + return false } - return true + return arePropsEqualFunc(args[0], args[1]) } - if (changeInPropsDetected) { - changeInPropsDetected = false - return false + if (!changeInPropsDetected) { + changeInPropsDetected = !arePropsEqualFunc(args[0], args[1]) } - return arePropsEqual(args[0], args[1]) + return true }) as F } @@ -123,7 +124,7 @@ function wrapWithShellContext( this.getOwnProps, boundShell ), - areOwnPropsEqual: wrapWithShouldUpdateExperimental( + areOwnPropsEqual: arePropsEqualFuncWrapper( shouldComponentUpdate, reduxConnectOptions.areOwnPropsEqual, this.getOwnProps, From ce21da11961d67c237ecb5ebe24da1db93b26a57 Mon Sep 17 00:00:00 2001 From: noa cohen Date: Mon, 15 Jul 2024 20:37:52 +0300 Subject: [PATCH 3/6] add test --- test/connectWithShell.spec.tsx | 113 +++++++++++++++++++++++++++++++++ 1 file changed, 113 insertions(+) diff --git a/test/connectWithShell.spec.tsx b/test/connectWithShell.spec.tsx index a390b72..75ee90f 100644 --- a/test/connectWithShell.spec.tsx +++ b/test/connectWithShell.spec.tsx @@ -256,6 +256,119 @@ describe('connectWithShell', () => { expect(ownPropsSpy).toHaveBeenCalledWith({ ownProp: true }) }) + it('should trigger recalculation of mergedProps with consideration of ownProps change once updates are permitted', () => { + const { host, shell, renderInShellContext } = createMocks(mockPackage) + + // Setup - create connected inner Component + interface InnerCompDispatchProps { + onClick(): void + } + interface InnerCompOwnProps { + num: number + } + type InnerCompProps = InnerCompOwnProps & InnerCompDispatchProps + const onClickSpy = jest.fn(num => {}) + const innerComponentRender = jest.fn() + const mapDispatchSpy = jest.fn() + let shouldUpdateInnerComp = false + + const mapDispatchToProps = (shell: Shell, state: unknown, ownProps?: InnerCompOwnProps): InnerCompDispatchProps => { + mapDispatchSpy() + return { + onClick: () => onClickSpy(ownProps?.num || 0) + } + } + + const PureInnerComp: FunctionComponent = ({ num, onClick }) => { + innerComponentRender() + return
{num.toString()}
+ } + + const ConnectedInnerComp = connectWithShell(undefined, mapDispatchToProps, shell, { + shouldComponentUpdate: () => shouldUpdateInnerComp, + allowOutOfEntryPoint: true + })(PureInnerComp) + + // Setup - create connected outer Component + interface OuterCompStateProps { + num: number + str: string + } + type OuterCompProps = OuterCompStateProps + + const mapStateSpy = jest.fn() + let stateProps: OuterCompStateProps = { num: 1, str: 'initialState' } + const mapStateToProps = (): OuterCompStateProps => { + mapStateSpy() + return stateProps + } + const outerComponentRenderSpy = jest.fn() + + const PureOuterComp: FunctionComponent = ({ num }) => { + outerComponentRenderSpy() + return + } + + const ConnectedOuterComp = connectWithShell(mapStateToProps, undefined, shell, { + allowOutOfEntryPoint: true + })(PureOuterComp) + + // SetUp - use a reducer that creates a new state for any dispatch action + let counter = 0 + host.getStore().replaceReducer(() => ({ + counter: ++counter + })) + + const updateOuterComp = (newStateProps: OuterCompStateProps) => { + stateProps = newStateProps + + act(() => { + host.getStore().dispatch({ type: '' }) + host.getStore().flush() + }) + } + + // Setup - render outer component + const { testKit } = renderInShellContext() + + if (!testKit) { + throw new Error('Connected component fail to render') + } + + expect(mapStateSpy).toHaveBeenCalledTimes(1) + expect(outerComponentRenderSpy).toHaveBeenCalledTimes(1) + expect(mapDispatchSpy).toHaveBeenCalledTimes(1) + expect(innerComponentRender).toHaveBeenCalledTimes(1) + expect(testKit.root.findByType(ConnectedInnerComp).find(x => typeof x.children[0] === 'string').children[0]).toBe('1') + testKit.root + .findByType(PureInnerComp) + .find(x => x.type === 'div') + .props.onClick() + expect(onClickSpy).toHaveBeenCalledWith(1) + + // Act - update outer component + updateOuterComp({ num: 2, str: 'nextState_1' }) + expect(mapStateSpy).toHaveBeenCalledTimes(2) + expect(outerComponentRenderSpy).toHaveBeenCalledTimes(2) + expect(mapDispatchSpy).toHaveBeenCalledTimes(1) + expect(innerComponentRender).toHaveBeenCalledTimes(1) + + // Act - allow updates for inner component, then update outer component + shouldUpdateInnerComp = true + updateOuterComp({ num: 2, str: 'nextState_2' }) + expect(mapStateSpy).toHaveBeenCalledTimes(3) + expect(outerComponentRenderSpy).toHaveBeenCalledTimes(3) + expect(mapDispatchSpy).toHaveBeenCalledTimes(2) + expect(innerComponentRender).toHaveBeenCalledTimes(2) + + expect(testKit.root.findByType(ConnectedInnerComp).find(x => typeof x.children[0] === 'string').children[0]).toBe('2') + testKit.root + .findByType(PureInnerComp) + .find(x => x.type === 'div') + .props.onClick() + expect(onClickSpy).toHaveBeenCalledWith(2) + }) + it('should pass scoped state to mapStateToProps', () => { const { host, shell, renderInShellContext } = createMocks(mockPackage) From 605a3047982ea9c0e7c963701a395120ff9c0334 Mon Sep 17 00:00:00 2001 From: noa cohen Date: Mon, 15 Jul 2024 20:42:20 +0300 Subject: [PATCH 4/6] improve code readability --- src/connectWithShell.tsx | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/connectWithShell.tsx b/src/connectWithShell.tsx index 1421fa4..f51de47 100644 --- a/src/connectWithShell.tsx +++ b/src/connectWithShell.tsx @@ -48,18 +48,18 @@ function arePropsEqualFuncWrapper) => { const componentShouldUpdate = componentShouldUpdateFunc(shell, getOwnProps()) if (componentShouldUpdate) { - if (changeInPropsDetected) { - changeInPropsDetected = false + if (hasPendingPropChanges) { + hasPendingPropChanges = false return false } return arePropsEqualFunc(args[0], args[1]) } - if (!changeInPropsDetected) { - changeInPropsDetected = !arePropsEqualFunc(args[0], args[1]) + if (!hasPendingPropChanges) { + hasPendingPropChanges = !arePropsEqualFunc(args[0], args[1]) } return true }) as F From f47fa1b3771dc57eb62261a914048acab0d74129 Mon Sep 17 00:00:00 2001 From: noa cohen Date: Tue, 16 Jul 2024 10:35:02 +0300 Subject: [PATCH 5/6] update test comments --- test/connectWithShell.spec.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/connectWithShell.spec.tsx b/test/connectWithShell.spec.tsx index 75ee90f..7f2b875 100644 --- a/test/connectWithShell.spec.tsx +++ b/test/connectWithShell.spec.tsx @@ -313,7 +313,7 @@ describe('connectWithShell', () => { allowOutOfEntryPoint: true })(PureOuterComp) - // SetUp - use a reducer that creates a new state for any dispatch action + // SetUp - use a reducer that creates a new state for any dispatched action let counter = 0 host.getStore().replaceReducer(() => ({ counter: ++counter @@ -346,7 +346,7 @@ describe('connectWithShell', () => { .props.onClick() expect(onClickSpy).toHaveBeenCalledWith(1) - // Act - update outer component + // Act - update outer component, while updates for inner component are blocked updateOuterComp({ num: 2, str: 'nextState_1' }) expect(mapStateSpy).toHaveBeenCalledTimes(2) expect(outerComponentRenderSpy).toHaveBeenCalledTimes(2) From bc017de7c653c1a9eeef9687f133d0bb15ffb454 Mon Sep 17 00:00:00 2001 From: noa cohen Date: Tue, 16 Jul 2024 14:36:02 +0300 Subject: [PATCH 6/6] improve tests --- test/connectWithShell.spec.tsx | 218 +++++++++++++++++++-------------- 1 file changed, 127 insertions(+), 91 deletions(-) diff --git a/test/connectWithShell.spec.tsx b/test/connectWithShell.spec.tsx index 7f2b875..ed2915f 100644 --- a/test/connectWithShell.spec.tsx +++ b/test/connectWithShell.spec.tsx @@ -14,7 +14,7 @@ import { TOGGLE_MOCK_VALUE, collectAllTexts } from '../testKit' -import { ReactTestRenderer, act, create } from 'react-test-renderer' +import { ReactTestRenderer, act, create, ReactTestInstance } from 'react-test-renderer' import { AnyAction } from 'redux' import { ObservedSelectorsMap, observeWithShell } from '../src' @@ -256,10 +256,7 @@ describe('connectWithShell', () => { expect(ownPropsSpy).toHaveBeenCalledWith({ ownProp: true }) }) - it('should trigger recalculation of mergedProps with consideration of ownProps change once updates are permitted', () => { - const { host, shell, renderInShellContext } = createMocks(mockPackage) - - // Setup - create connected inner Component + describe('arePropsEqualFuncWrapper', () => { interface InnerCompDispatchProps { onClick(): void } @@ -267,106 +264,145 @@ describe('connectWithShell', () => { num: number } type InnerCompProps = InnerCompOwnProps & InnerCompDispatchProps - const onClickSpy = jest.fn(num => {}) - const innerComponentRender = jest.fn() - const mapDispatchSpy = jest.fn() - let shouldUpdateInnerComp = false - - const mapDispatchToProps = (shell: Shell, state: unknown, ownProps?: InnerCompOwnProps): InnerCompDispatchProps => { - mapDispatchSpy() - return { - onClick: () => onClickSpy(ownProps?.num || 0) - } - } - - const PureInnerComp: FunctionComponent = ({ num, onClick }) => { - innerComponentRender() - return
{num.toString()}
- } - - const ConnectedInnerComp = connectWithShell(undefined, mapDispatchToProps, shell, { - shouldComponentUpdate: () => shouldUpdateInnerComp, - allowOutOfEntryPoint: true - })(PureInnerComp) - - // Setup - create connected outer Component interface OuterCompStateProps { num: number str: string } type OuterCompProps = OuterCompStateProps - const mapStateSpy = jest.fn() - let stateProps: OuterCompStateProps = { num: 1, str: 'initialState' } - const mapStateToProps = (): OuterCompStateProps => { - mapStateSpy() - return stateProps - } - const outerComponentRenderSpy = jest.fn() + // spies + let mapDispatchInnerCompSpy: jest.Mock + let innerComponentRender: jest.Mock + let mapStateOuterCompSpy: jest.Mock + let outerComponentRenderSpy: jest.Mock + let innerCompOnClickSpy: jest.Mock + let innerCompShouldComponentUpdateSpy: jest.Mock + + // action helpers + let shouldUpdateInnerComp: boolean + let updateOuterComp: (newStateProps: OuterCompStateProps) => void + + // assertion helpers + let getInnerCompText: () => ReactTestInstance | string + let invokeInnerCompOnClick: () => void + + beforeEach(() => { + const { host, shell, renderInShellContext } = createMocks(mockPackage) + + // Setup - create connected inner Component + innerComponentRender = jest.fn() + mapDispatchInnerCompSpy = jest.fn() + shouldUpdateInnerComp = false + innerCompOnClickSpy = jest.fn(num => {}) + innerCompShouldComponentUpdateSpy = jest.fn((ownProps: InnerCompOwnProps) => {}) + + const mapDispatchToProps = (shell: Shell, state: unknown, ownProps?: InnerCompOwnProps): InnerCompDispatchProps => { + mapDispatchInnerCompSpy() + return { + onClick: () => innerCompOnClickSpy(ownProps?.num || 0) + } + } - const PureOuterComp: FunctionComponent = ({ num }) => { - outerComponentRenderSpy() - return - } + const PureInnerComp: FunctionComponent = ({ num, onClick }) => { + innerComponentRender() + return
{num.toString()}
+ } - const ConnectedOuterComp = connectWithShell(mapStateToProps, undefined, shell, { - allowOutOfEntryPoint: true - })(PureOuterComp) + const ConnectedInnerComp = connectWithShell(undefined, mapDispatchToProps, shell, { + shouldComponentUpdate: (shell, nextOwnProps) => { + innerCompShouldComponentUpdateSpy(nextOwnProps) + return shouldUpdateInnerComp + }, + allowOutOfEntryPoint: true + })(PureInnerComp) + + // Setup - create connected outer Component + mapStateOuterCompSpy = jest.fn() + outerComponentRenderSpy = jest.fn() + + let stateProps: OuterCompStateProps = { num: 1, str: 'initialState' } + const mapStateToProps = (): OuterCompStateProps => { + mapStateOuterCompSpy() + return stateProps + } - // SetUp - use a reducer that creates a new state for any dispatched action - let counter = 0 - host.getStore().replaceReducer(() => ({ - counter: ++counter - })) + const PureOuterComp: FunctionComponent = ({ num }) => { + outerComponentRenderSpy() + return + } - const updateOuterComp = (newStateProps: OuterCompStateProps) => { - stateProps = newStateProps + const ConnectedOuterComp = connectWithShell(mapStateToProps, undefined, shell, { + allowOutOfEntryPoint: true + })(PureOuterComp) - act(() => { - host.getStore().dispatch({ type: '' }) - host.getStore().flush() - }) - } + updateOuterComp = (newStateProps: OuterCompStateProps) => { + stateProps = newStateProps - // Setup - render outer component - const { testKit } = renderInShellContext() + act(() => { + host.getStore().dispatch({ type: '' }) + host.getStore().flush() + }) + } - if (!testKit) { - throw new Error('Connected component fail to render') - } + // SetUp - use a reducer that creates a new state for any dispatched action + let counter = 0 + host.getStore().replaceReducer(() => ({ + counter: ++counter + })) - expect(mapStateSpy).toHaveBeenCalledTimes(1) - expect(outerComponentRenderSpy).toHaveBeenCalledTimes(1) - expect(mapDispatchSpy).toHaveBeenCalledTimes(1) - expect(innerComponentRender).toHaveBeenCalledTimes(1) - expect(testKit.root.findByType(ConnectedInnerComp).find(x => typeof x.children[0] === 'string').children[0]).toBe('1') - testKit.root - .findByType(PureInnerComp) - .find(x => x.type === 'div') - .props.onClick() - expect(onClickSpy).toHaveBeenCalledWith(1) - - // Act - update outer component, while updates for inner component are blocked - updateOuterComp({ num: 2, str: 'nextState_1' }) - expect(mapStateSpy).toHaveBeenCalledTimes(2) - expect(outerComponentRenderSpy).toHaveBeenCalledTimes(2) - expect(mapDispatchSpy).toHaveBeenCalledTimes(1) - expect(innerComponentRender).toHaveBeenCalledTimes(1) - - // Act - allow updates for inner component, then update outer component - shouldUpdateInnerComp = true - updateOuterComp({ num: 2, str: 'nextState_2' }) - expect(mapStateSpy).toHaveBeenCalledTimes(3) - expect(outerComponentRenderSpy).toHaveBeenCalledTimes(3) - expect(mapDispatchSpy).toHaveBeenCalledTimes(2) - expect(innerComponentRender).toHaveBeenCalledTimes(2) - - expect(testKit.root.findByType(ConnectedInnerComp).find(x => typeof x.children[0] === 'string').children[0]).toBe('2') - testKit.root - .findByType(PureInnerComp) - .find(x => x.type === 'div') - .props.onClick() - expect(onClickSpy).toHaveBeenCalledWith(2) + // Setup - render outer component + const { testKit } = renderInShellContext() + + if (!testKit) { + throw new Error('Connected component fail to render') + } + + // create assertion helpers + getInnerCompText = () => testKit.root.findByType(ConnectedInnerComp).find(x => typeof x.children[0] === 'string').children[0] + invokeInnerCompOnClick = () => + testKit.root + .findByType(PureInnerComp) + .find(x => x.type === 'div') + .props.onClick() + }) + it('should execute mapDispatchToProps, mapStateToProps, and render for both components during mount phase', () => { + // Assert initial execution of mapping functions and components render + expect(mapStateOuterCompSpy).toHaveBeenCalledTimes(1) + expect(outerComponentRenderSpy).toHaveBeenCalledTimes(1) + expect(mapDispatchInnerCompSpy).toHaveBeenCalledTimes(1) + expect(innerComponentRender).toHaveBeenCalledTimes(1) + expect(getInnerCompText()).toBe('1') + invokeInnerCompOnClick() + expect(innerCompOnClickSpy).toHaveBeenCalledWith(1) + }) + it('should not trigger mapDispatchToProps or re-render the inner component when ownProps change while updates are blocked for the inner component', () => { + // Act - update outer component, while updates for inner component are blocked + updateOuterComp({ num: 2, str: 'nextState_1' }) + + // Assert - outer component re-rendered and passed new ownProps to inner component + expect(mapStateOuterCompSpy).toHaveBeenCalledTimes(2) + expect(outerComponentRenderSpy).toHaveBeenCalledTimes(2) + expect(innerCompShouldComponentUpdateSpy).toHaveBeenCalledWith({ num: 2 }) + + // Assert - should not trigger mapDispatchToProps or re-render of inner component even though it's ownProps have changed + expect(mapDispatchInnerCompSpy).toHaveBeenCalledTimes(1) + expect(innerComponentRender).toHaveBeenCalledTimes(1) + }) + it('should trigger recalculation of mergedProps with consideration of ownProps change once updates are permitted', () => { + // Act - update outer component, while updates for inner component are blocked + updateOuterComp({ num: 2, str: 'nextState_1' }) + + // Act - allow updates for inner component, then update outer component + shouldUpdateInnerComp = true + updateOuterComp({ num: 2, str: 'nextState_2' }) + + // Assert - mapDispatchToProps and re-render of inner component were triggered + expect(mapDispatchInnerCompSpy).toHaveBeenCalledTimes(2) + expect(innerComponentRender).toHaveBeenCalledTimes(2) + expect(getInnerCompText()).toBe('2') + invokeInnerCompOnClick() + expect(innerCompOnClickSpy).toHaveBeenCalledWith(2) + }) }) it('should pass scoped state to mapStateToProps', () => {