From b7c693f90851ecd14b478bd58915a4be812f2fbf Mon Sep 17 00:00:00 2001 From: David Gilbertson Date: Tue, 10 Mar 2020 13:39:41 +1100 Subject: [PATCH] 82 Fix component did update (#84) Fixes #82 --- src/collect.tsx | 8 +- src/proxyHandlers.ts | 53 ++----- src/shared/debug.ts | 44 ++++++ src/updating.ts | 9 +- tests/integration/componentDidUpdate.test.tsx | 129 ++++++++++++++---- 5 files changed, 167 insertions(+), 76 deletions(-) diff --git a/src/collect.tsx b/src/collect.tsx index 5eba915..a6ba71f 100644 --- a/src/collect.tsx +++ b/src/collect.tsx @@ -5,6 +5,10 @@ import state from './shared/state'; import { debug } from './shared/debug'; import { CollectorComponent, Store, WithStoreProp } from './shared/types'; +// As we render down into a tree of collected components, we will start/stop +// recording +const componentStack: CollectorComponent[] = []; + const startRecordingGetsForComponent = (component: CollectorComponent) => { if (!state.isInBrowser) return; @@ -15,6 +19,7 @@ const startRecordingGetsForComponent = (component: CollectorComponent) => { }); state.currentComponent = component; + componentStack.push(state.currentComponent); }; const stopRecordingGetsForComponent = () => { @@ -24,7 +29,8 @@ const stopRecordingGetsForComponent = () => { console.groupEnd(); }); - state.currentComponent = null; + componentStack.pop(); + state.currentComponent = componentStack[componentStack.length - 1] || null; }; type RemoveStore = Pick>; diff --git a/src/proxyHandlers.ts b/src/proxyHandlers.ts index 5c2fa4e..a0e8c0d 100644 --- a/src/proxyHandlers.ts +++ b/src/proxyHandlers.ts @@ -1,5 +1,5 @@ import { getFromNextStore, updateInNextStore } from './store'; -import { debug } from './shared/debug'; +import { logGet, logSet, logDelete } from './shared/debug'; import state from './shared/state'; import * as utils from './shared/utils'; import * as paths from './shared/paths'; @@ -34,46 +34,6 @@ const addListener = (propPath: PropPath) => { state.listeners.set(pathString, components); }; -/** - * Is this an attempt to get something from the store outside the render cycle? - * This might be store.tasks.push() in a click event right after doing store.tasks = [] - * In this case, we should always return from nextStore. - * @see setStoreTwiceInOnClick.test.js - */ -const isGettingPropOutsideOfRenderCycle = (prop: any) => - !state.currentComponent && - !utils.isSymbol(prop) && - prop !== 'constructor' && - !state.proxyIsMuted; - -const logSet = (target: Target, prop: any, value?: any) => { - debug(() => { - console.groupCollapsed(`SET: ${paths.extendToUserString(target, prop)}`); - console.info('From:', utils.getValue(target, prop)); - console.info('To: ', value); - console.groupEnd(); - }); -}; - -const logGet = (target: Target, prop?: any, value?: any) => { - debug(() => { - console.groupCollapsed(`GET: ${paths.extendToUserString(target, prop)}`); - console.info(`Component: <${state.currentComponent!._name}>`); - if (typeof value !== 'undefined') { - console.info('Value:', value); - } - console.groupEnd(); - }); -}; - -const logDelete = (target: Target, prop: any) => { - debug(() => { - console.groupCollapsed(`DELETE: ${paths.extendToUserString(target, prop)}`); - console.info('Property: ', paths.extendToUserString(target, prop)); - console.groupEnd(); - }); -}; - /** * We have different handlers (different traps) for object/array and map/set. */ @@ -214,7 +174,16 @@ export const getHandlerForObject = ( } // TODO (davidg): does 'size' need to be below this? Would I be getting the wrong size? - if (isGettingPropOutsideOfRenderCycle(prop)) { + // Is this an attempt to get something from the store outside the + // render cycle? This might be store.tasks.push() in a click event + // right after `doing store.tasks = []` + // In this case, we should always return from nextStore. + // See setStoreTwiceInOnClick.test.js + if ( + !utils.isSymbol(prop) && + prop !== 'constructor' && + !state.proxyIsMuted + ) { return getFromNextStore(target, prop); } diff --git a/src/shared/debug.ts b/src/shared/debug.ts index d609cfa..4f8ddc4 100644 --- a/src/shared/debug.ts +++ b/src/shared/debug.ts @@ -1,3 +1,8 @@ +import { CollectorComponent, Target } from './types'; +import * as paths from './paths'; +import * as utils from './utils'; +import state from './state'; + const DEBUG_ON = 'on'; const DEBUG_OFF = 'off'; @@ -26,3 +31,42 @@ export const debugOff = () => { export const debug = (cb: () => void) => { if (DEBUG === DEBUG_ON) cb(); }; + +export const logGet = (target: Target, prop?: any, value?: any) => { + debug(() => { + console.groupCollapsed(`GET: ${paths.extendToUserString(target, prop)}`); + console.info(`Component: <${state.currentComponent!._name}>`); + if (typeof value !== 'undefined') { + console.info('Value:', value); + } + console.groupEnd(); + }); +}; + +export const logSet = (target: Target, prop: any, value?: any) => { + debug(() => { + console.groupCollapsed(`SET: ${paths.extendToUserString(target, prop)}`); + console.info('From:', utils.getValue(target, prop)); + console.info('To: ', value); + console.groupEnd(); + }); +}; + +export const logDelete = (target: Target, prop: any) => { + debug(() => { + console.groupCollapsed(`DELETE: ${paths.extendToUserString(target, prop)}`); + console.info('Property: ', paths.extendToUserString(target, prop)); + console.groupEnd(); + }); +}; + +export const logUpdate = ( + component: CollectorComponent, + propsUpdated: string[] +) => { + debug(() => { + console.groupCollapsed(`UPDATE: <${component._name}>`); + console.info('Changed properties:', propsUpdated); + console.groupEnd(); + }); +}; diff --git a/src/updating.ts b/src/updating.ts index 6fc7191..2d1ab4f 100644 --- a/src/updating.ts +++ b/src/updating.ts @@ -1,4 +1,4 @@ -import { debug } from './shared/debug'; +import { logUpdate } from './shared/debug'; import state from './shared/state'; import * as paths from './shared/paths'; import * as utils from './shared/utils'; @@ -28,11 +28,7 @@ const flushUpdates = () => { queue.timeoutPending = false; queue.components.forEach((propsUpdated, component) => { - debug(() => { - console.groupCollapsed(`UPDATE: <${component._name}>`); - console.info('Changed properties:', Array.from(propsUpdated)); - console.groupEnd(); - }); + logUpdate(component, Array.from(propsUpdated)); component.update(); }); @@ -50,7 +46,6 @@ const flushUpdates = () => { queue.changedPaths.clear(); state.proxyIsMuted = true; - // TODO (davidg): rename to prevStore/store utils.replaceObject(state.store, state.nextStore); state.proxyIsMuted = false; }; diff --git a/tests/integration/componentDidUpdate.test.tsx b/tests/integration/componentDidUpdate.test.tsx index 03a8875..554e4f5 100644 --- a/tests/integration/componentDidUpdate.test.tsx +++ b/tests/integration/componentDidUpdate.test.tsx @@ -1,48 +1,125 @@ +// eslint-disable-next-line max-classes-per-file import React, { Component } from 'react'; import { render } from '@testing-library/react'; -import { collect, store, WithStoreProp } from '../../src'; +import { collect, store as globalStore, WithStoreProp } from '../../src'; interface Props extends WithStoreProp { - fetchData: () => {}; + reportUserChange: (comp: string, prev: number, next: number) => {}; } -class RawClassComponent extends Component { - componentDidUpdate(prevProps: Props) { - if (this.props.store.userId !== prevProps.store.userId) { - this.props.fetchData(); +// This test has multiple collected components to test a specific scenario +// https://github.com/davidgilbertson/react-recollect/issues/82 + +const LevelTwo = collect( + class LevelTwoRaw extends Component { + componentDidUpdate(prevProps: Props) { + this.props.reportUserChange( + 'LevelTwo', + prevProps.store.userId, + this.props.store.userId + ); + } + + render() { + return

User ID: {this.props.store.userId}

; } } +); + +const LevelOneA = collect( + class LevelOneARaw extends Component { + componentDidUpdate(prevProps: Props) { + this.props.reportUserChange( + 'LevelOneA', + prevProps.store.userId, + this.props.store.userId + ); + } - render() { - return ( -
-

User ID: {this.props.store.userId}

- -
- ); + render() { + return ( +
+

User ID: {this.props.store.userId}

+ + +
+ ); + } } -} +); -const ClassComponent = collect(RawClassComponent); +const LevelOneB = collect( + class LevelOneBRaw extends Component { + componentDidUpdate(prevProps: Props) { + this.props.reportUserChange( + 'LevelOneB', + prevProps.store.userId, + this.props.store.userId + ); + } -const fetchData = jest.fn(); + render() { + return

User ID: {this.props.store.userId}

; + } + } +); + +const ParentComponent = collect( + class ParentComponentRaw extends Component { + componentDidUpdate(prevProps: Props) { + this.props.reportUserChange( + 'ParentComponentRaw', + prevProps.store.userId, + this.props.store.userId + ); + } + + render() { + return ( +
+

User ID: {this.props.store.userId}

+ + + + +
+ ); + } + } +); + +const reportUserChange = jest.fn(); it('should handle a change in a value', () => { - store.userId = 1; + globalStore.userId = 1; - const { getByText } = render(); + const { getByText } = render( + + ); - expect(fetchData).toHaveBeenCalledTimes(0); + expect(reportUserChange).toHaveBeenCalledTimes(0); getByText('Switch user').click(); - expect(fetchData).toHaveBeenCalledTimes(1); + expect(reportUserChange).toHaveBeenCalledTimes(4); + + expect(reportUserChange).toHaveBeenNthCalledWith(1, 'LevelTwo', 1, 2); + expect(reportUserChange).toHaveBeenNthCalledWith(2, 'LevelOneA', 1, 2); + expect(reportUserChange).toHaveBeenNthCalledWith(3, 'LevelOneB', 1, 2); + expect(reportUserChange).toHaveBeenNthCalledWith( + 4, + 'ParentComponentRaw', + 1, + 2 + ); }); // it should listen for changes on props not called in the render() method.