From 96b30b767d97389c2e33b288959b87c152ddf7ad Mon Sep 17 00:00:00 2001 From: vzaidman Date: Mon, 9 Sep 2019 19:44:12 +0300 Subject: [PATCH 1/3] #50 - added tests that prove memo of class components fails --- src/patches/patchMemoComponent.test.js | 68 ++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/src/patches/patchMemoComponent.test.js b/src/patches/patchMemoComponent.test.js index 52842bf..f59435a 100644 --- a/src/patches/patchMemoComponent.test.js +++ b/src/patches/patchMemoComponent.test.js @@ -151,3 +151,71 @@ test('memo a forward ref component', () => { hookDifferences: false }) }) + +test('memo a class component', () => { + class ClassComponent extends React.Component{ + render(){ + return
hi!
+ } + } + + const MyComponent = React.memo(ClassComponent) + + MyComponent.whyDidYouRender = true + + const {rerender} = rtl.render( + + ) + + rerender( + + ) + + expect(updateInfos).toHaveLength(1) + expect(updateInfos[0].reason).toEqual({ + propsDifferences: [ + { + pathString: 'a', + diffType: diffTypes.deepEquals, + prevValue: [], + nextValue: [] + } + ], + stateDifferences: false, + hookDifferences: false + }) +}) + +test('memo a pure class component', () => { + class ClassComponent extends React.PureComponent{ + render(){ + return
hi!
+ } + } + + const MyComponent = React.memo(ClassComponent) + + MyComponent.whyDidYouRender = true + + const {rerender} = rtl.render( + + ) + + rerender( + + ) + + expect(updateInfos).toHaveLength(1) + expect(updateInfos[0].reason).toEqual({ + propsDifferences: [ + { + pathString: 'a', + diffType: diffTypes.deepEquals, + prevValue: [], + nextValue: [] + } + ], + stateDifferences: false, + hookDifferences: false + }) +}) From 772e6deaacdc876e3676b2eda25d94fd18740d96 Mon Sep 17 00:00:00 2001 From: vzaidman Date: Fri, 13 Sep 2019 11:59:31 +0300 Subject: [PATCH 2/3] using "@welldone-software/jest-console-handler" --- jestSetup.js | 2 +- package.json | 1 + testUtils/errorOnConsoleOutput.js | 36 ------------------------------- yarn.lock | 5 +++++ 4 files changed, 7 insertions(+), 37 deletions(-) delete mode 100644 testUtils/errorOnConsoleOutput.js diff --git a/jestSetup.js b/jestSetup.js index 7dd9684..d80873a 100644 --- a/jestSetup.js +++ b/jestSetup.js @@ -1,3 +1,3 @@ -import errorOnConsoleOutput from './testUtils/errorOnConsoleOutput' +import {errorOnConsoleOutput} from '@welldone-software/jest-console-handler' global.flushConsoleOutput = errorOnConsoleOutput() diff --git a/package.json b/package.json index c6ecdba..60ddda6 100644 --- a/package.json +++ b/package.json @@ -68,6 +68,7 @@ "@testing-library/jest-dom": "^4.1.0", "@testing-library/react": "^9.1.4", "@types/react": "^16.9.2", + "@welldone-software/jest-console-handler": "^0.1.0", "acorn-walk": "^7.0.0", "astring": "^1.4.1", "babel-core": "^7.0.0-bridge.0", diff --git a/testUtils/errorOnConsoleOutput.js b/testUtils/errorOnConsoleOutput.js deleted file mode 100644 index 8e64263..0000000 --- a/testUtils/errorOnConsoleOutput.js +++ /dev/null @@ -1,36 +0,0 @@ -export class UnexpectedConsoleOutput extends Error{ - constructor(message, consoleMessages){ - super(`${message}: ${JSON.stringify(consoleMessages, null, 2)}`) - this.consoleMessages = consoleMessages - } -} - -export default function errorOnConsoleOutput(){ - let consoleMessages = [] - - beforeEach(() => { - Object.keys(global.console) - .filter(consoleFnName => { - return consoleFnName.charAt(0) !== '_' && typeof(global.console[consoleFnName]) === 'function' - }) - .forEach(consoleFnName => { - global.console[consoleFnName] = (...args) => { - consoleMessages.push({level: consoleFnName, args}) - } - }) - }) - - afterEach(() => { - if(consoleMessages.length > 0){ - throw new UnexpectedConsoleOutput('Unhandled console messages in test', consoleMessages) - } - }) - - const flushConsoleMessages = () => { - const consoleMessagesToTake = consoleMessages - consoleMessages = [] - return consoleMessagesToTake - } - - return flushConsoleMessages -} diff --git a/yarn.lock b/yarn.lock index 1e88f17..fee8c3f 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1116,6 +1116,11 @@ lodash.unescape "4.0.1" semver "5.5.0" +"@welldone-software/jest-console-handler@^0.1.0": + version "0.1.0" + resolved "https://registry.yarnpkg.com/@welldone-software/jest-console-handler/-/jest-console-handler-0.1.0.tgz#20c3f5bcdc880a05e8915f8a029e135e0fb5e900" + integrity sha512-vH3a/oa78kHTUTiCnmMRWT1fPYxrEddXVoq+Y0aH9i1ArB3bQkM1/mzOnbgNpaXRIqpepduLjK8jNurCVbKWxw== + abab@^2.0.0: version "2.0.1" resolved "https://registry.yarnpkg.com/abab/-/abab-2.0.1.tgz#3fa17797032b71410ec372e11668f4b4ffc86a82" From a18cae61669d0257c50369f2409c3edad54c8de8 Mon Sep 17 00:00:00 2001 From: vzaidman Date: Fri, 13 Sep 2019 12:00:19 +0300 Subject: [PATCH 3/3] fix #50 - support memoization of class components --- src/patches/patchForwardRefComponent.js | 39 ++------- src/patches/patchForwardRefComponent.test.js | 92 ++++++++++---------- src/patches/patchFunctionalComponent.js | 21 +++-- src/patches/patchMemoComponent.js | 47 ++++------ src/patches/patchMemoComponent.test.js | 1 + src/utils.js | 14 +++ src/whyDidYouRender.js | 14 +-- 7 files changed, 106 insertions(+), 122 deletions(-) diff --git a/src/patches/patchForwardRefComponent.js b/src/patches/patchForwardRefComponent.js index 3412826..c5381d2 100644 --- a/src/patches/patchForwardRefComponent.js +++ b/src/patches/patchForwardRefComponent.js @@ -1,40 +1,19 @@ import {defaults} from 'lodash' -import getUpdateInfo from '../getUpdateInfo' import getDisplayName from '../getDisplayName' -import {REACT_MEMO_TYPE} from '../consts' +import {isMemoComponent} from '../utils' +import patchFunctionalComponent from './patchFunctionalComponent' export default function patchForwardRefComponent(ForwardRefComponent, displayName, React, options){ const {render: InnerForwardRefComponent} = ForwardRefComponent - const isInnerComponentMemoized = InnerForwardRefComponent.$$typeof === REACT_MEMO_TYPE - const WrappedFunctionalComponent = isInnerComponentMemoized ? InnerForwardRefComponent.type : InnerForwardRefComponent - - function WDYRWrappedByReactForwardRefFunctionalComponent(){ - const nextProps = arguments[0] - const ref = React.useRef() - - const prevProps = ref.current - ref.current = nextProps - - if(prevProps){ - const notification = getUpdateInfo({ - Component: ForwardRefComponent, - displayName, - prevProps, - nextProps, - options - }) - - // if a memoized functional component re-rendered without props change / prop values change - // it was probably caused by a hook and we should not care about it - if(notification.reason.propsDifferences && notification.reason.propsDifferences.length > 0){ - options.notifier(notification) - } - } - - return WrappedFunctionalComponent(...arguments) - } + const isInnerComponentMemoized = isMemoComponent(InnerForwardRefComponent) + const WrappedFunctionalComponent = isInnerComponentMemoized ? + InnerForwardRefComponent.type : InnerForwardRefComponent + + const WDYRWrappedByReactForwardRefFunctionalComponent = ( + patchFunctionalComponent(WrappedFunctionalComponent, isInnerComponentMemoized, displayName, React, options) + ) WDYRWrappedByReactForwardRefFunctionalComponent.displayName = getDisplayName(WrappedFunctionalComponent) WDYRWrappedByReactForwardRefFunctionalComponent.ComponentForHooksTracking = WrappedFunctionalComponent diff --git a/src/patches/patchForwardRefComponent.test.js b/src/patches/patchForwardRefComponent.test.js index 8917c8a..d390aab 100644 --- a/src/patches/patchForwardRefComponent.test.js +++ b/src/patches/patchForwardRefComponent.test.js @@ -63,49 +63,51 @@ test('forward ref', () => { }) test('forward ref a memo component', () => { - /* turns out this is not supported by react at this point. */ - - // const content = 'My component!!!' - // - // const MyComponent = React.forwardRef(React.memo((props, ref) => { - // return
{content}
- // }, () => true)) - // - // MyComponent.whyDidYouRender = true - // - // let componentContentFromRef = null - // let timesRefWasCalled = 0 - // - // const handleRef = ref => { - // if(!ref){ - // return - // } - // timesRefWasCalled++ - // componentContentFromRef = ref.innerHTML - // } - // - // const {rerender} = rtl.render( - // - // ) - // - // rerender( - // - // ) - // - // expect(componentContentFromRef).toBe(content) - // expect(timesRefWasCalled).toBe(1) - // - // expect(updateInfos).toHaveLength(1) - // expect(updateInfos[0].reason).toEqual({ - // propsDifferences: [ - // { - // pathString: 'a', - // diffType: diffTypes.deepEquals, - // prevValue: [], - // nextValue: [] - // } - // ], - // stateDifferences: false, - // hookDifferences: false - // }) + // This is not supported by React 16.9 + expect(() => { + const content = 'My component!!!' + + const MyComponent = React.forwardRef(React.memo((props, ref) => { + return
{content}
+ }, () => true)) + + MyComponent.whyDidYouRender = true + + let componentContentFromRef = null + let timesRefWasCalled = 0 + + const handleRef = ref => { + if(!ref){ + return + } + timesRefWasCalled++ + componentContentFromRef = ref.innerHTML + } + + const {rerender} = rtl.render( + + ) + + rerender( + + ) + + expect(componentContentFromRef).toBe(content) + expect(timesRefWasCalled).toBe(1) + + expect(updateInfos).toHaveLength(1) + expect(updateInfos[0].reason).toEqual({ + propsDifferences: [ + { + pathString: 'a', + diffType: diffTypes.deepEquals, + prevValue: [], + nextValue: [] + } + ], + stateDifferences: false, + hookDifferences: false + }) + }).toThrow() + global.flushConsoleOutput() }) diff --git a/src/patches/patchFunctionalComponent.js b/src/patches/patchFunctionalComponent.js index e583b0b..a53ee15 100644 --- a/src/patches/patchFunctionalComponent.js +++ b/src/patches/patchFunctionalComponent.js @@ -2,15 +2,16 @@ import {defaults} from 'lodash' import getUpdateInfo from '../getUpdateInfo' -export default function patchFunctionalComponent(FunctionalComponent, displayName, React, options){ - function WDYRFunctionalComponent(nextProps){ +export default function patchFunctionalComponent(FunctionalComponent, isPure, displayName, React, options){ + function WDYRFunctionalComponent(){ + const nextProps = arguments[0] const ref = React.useRef() const prevProps = ref.current ref.current = nextProps if(prevProps){ - const notification = getUpdateInfo({ + const updateInfo = getUpdateInfo({ Component: FunctionalComponent, displayName, prevProps, @@ -18,14 +19,18 @@ export default function patchFunctionalComponent(FunctionalComponent, displayNam options }) - // if a functional component re-rendered without a props change - // it was probably caused by a hook and we should not care about it - if(notification.reason.propsDifferences){ - options.notifier(notification) + const shouldNotify = ( + updateInfo.reason.propsDifferences && ( + !(isPure && updateInfo.reason.propsDifferences.length === 0) + ) + ) + + if(shouldNotify){ + options.notifier(updateInfo) } } - return FunctionalComponent(nextProps) + return FunctionalComponent(...arguments) } WDYRFunctionalComponent.displayName = displayName diff --git a/src/patches/patchMemoComponent.js b/src/patches/patchMemoComponent.js index 2081ded..57c03dc 100644 --- a/src/patches/patchMemoComponent.js +++ b/src/patches/patchMemoComponent.js @@ -1,48 +1,31 @@ import {defaults} from 'lodash' -import getUpdateInfo from '../getUpdateInfo' import getDisplayName from '../getDisplayName' -import {REACT_FORWARD_REF_TYPE} from '../consts' +import {isForwardRefComponent, isReactClassComponent} from '../utils' +import patchClassComponent from './patchClassComponent' +import patchFunctionalComponent from './patchFunctionalComponent' export default function patchMemoComponent(MemoComponent, displayName, React, options){ const {type: InnerMemoComponent} = MemoComponent - const isInnerMemoComponentForwardRefs = InnerMemoComponent.$$typeof === REACT_FORWARD_REF_TYPE - const WrappedFunctionalComponent = isInnerMemoComponentForwardRefs ? InnerMemoComponent.render : InnerMemoComponent + const isInnerMemoComponentAClassComponent = isReactClassComponent(InnerMemoComponent) + const isInnerMemoComponentForwardRefs = isForwardRefComponent(InnerMemoComponent) - function WDYRWrappedByMemoFunctionalComponent(){ - const nextProps = arguments[0] - const ref = React.useRef() + const WrappedFunctionalComponent = isInnerMemoComponentForwardRefs ? + InnerMemoComponent.render : + InnerMemoComponent - const prevProps = ref.current - ref.current = nextProps + const PatchedInnerComponent = isInnerMemoComponentAClassComponent ? + patchClassComponent(WrappedFunctionalComponent, displayName, React, options) : + patchFunctionalComponent(WrappedFunctionalComponent, true, displayName, React, options) - if(prevProps){ - const notification = getUpdateInfo({ - Component: MemoComponent, - displayName, - prevProps, - nextProps, - options - }) - - // if a memoized functional component re-rendered without props change / prop values change - // it was probably caused by a hook and we should not care about it - if(notification.reason.propsDifferences && notification.reason.propsDifferences.length > 0){ - options.notifier(notification) - } - } - - return WrappedFunctionalComponent(...arguments) - } - - WDYRWrappedByMemoFunctionalComponent.displayName = getDisplayName(WrappedFunctionalComponent) - WDYRWrappedByMemoFunctionalComponent.ComponentForHooksTracking = MemoComponent - defaults(WDYRWrappedByMemoFunctionalComponent, WrappedFunctionalComponent) + PatchedInnerComponent.displayName = getDisplayName(WrappedFunctionalComponent) + PatchedInnerComponent.ComponentForHooksTracking = MemoComponent + defaults(PatchedInnerComponent, WrappedFunctionalComponent) const WDYRMemoizedFunctionalComponent = React.memo( - isInnerMemoComponentForwardRefs ? React.forwardRef(WDYRWrappedByMemoFunctionalComponent) : WDYRWrappedByMemoFunctionalComponent, + isInnerMemoComponentForwardRefs ? React.forwardRef(PatchedInnerComponent) : PatchedInnerComponent, MemoComponent.compare ) diff --git a/src/patches/patchMemoComponent.test.js b/src/patches/patchMemoComponent.test.js index f59435a..3e9de9c 100644 --- a/src/patches/patchMemoComponent.test.js +++ b/src/patches/patchMemoComponent.test.js @@ -218,4 +218,5 @@ test('memo a pure class component', () => { stateDifferences: false, hookDifferences: false }) + global.flushConsoleOutput() }) diff --git a/src/utils.js b/src/utils.js index 40506a5..0cb03e0 100644 --- a/src/utils.js +++ b/src/utils.js @@ -1,4 +1,6 @@ // copied from https://github.com/facebook/react/blob/master/packages/react-reconciler/src/ReactTypeOfMode.js +import {REACT_FORWARD_REF_TYPE, REACT_MEMO_TYPE} from './consts' + const StrictMode = 0b0001 // based on "findStrictRoot" from https://github.com/facebook/react/blob/master/packages/react-reconciler/src/ReactStrictModeWarnings.js @@ -13,3 +15,15 @@ export function checkIfInsideAStrictModeTree(reactComponentInstance){ } return false } + +export function isReactClassComponent(Component){ + return Component.prototype && !!Component.prototype.isReactComponent +} + +export function isMemoComponent(Component){ + return Component.$$typeof === REACT_MEMO_TYPE +} + +export function isForwardRefComponent(Component){ + return Component.$$typeof === REACT_FORWARD_REF_TYPE +} diff --git a/src/whyDidYouRender.js b/src/whyDidYouRender.js index 2c2bafa..b831000 100644 --- a/src/whyDidYouRender.js +++ b/src/whyDidYouRender.js @@ -10,7 +10,7 @@ import patchFunctionalComponent from './patches/patchFunctionalComponent' import patchMemoComponent from './patches/patchMemoComponent' import patchForwardRefComponent from './patches/patchForwardRefComponent' -import {REACT_MEMO_TYPE, REACT_FORWARD_REF_TYPE} from './consts' +import {isForwardRefComponent, isMemoComponent, isReactClassComponent} from './utils' function trackHookChanges(hookName, {path: hookPath}, hookResult, React, options){ const nextHook = hookResult @@ -52,19 +52,19 @@ function trackHookChanges(hookName, {path: hookPath}, hookResult, React, options } function createPatchedComponent(componentsMap, Component, displayName, React, options){ - if(Component.$$typeof === REACT_MEMO_TYPE){ + if(isMemoComponent(Component)){ return patchMemoComponent(Component, displayName, React, options) } - if(Component.$$typeof === REACT_FORWARD_REF_TYPE){ + if(isForwardRefComponent(Component)){ return patchForwardRefComponent(Component, displayName, React, options) } - if(Component.prototype && Component.prototype.isReactComponent){ + if(isReactClassComponent(Component)){ return patchClassComponent(Component, displayName, React, options) } - return patchFunctionalComponent(Component, displayName, React, options) + return patchFunctionalComponent(Component, false, displayName, React, options) } function getPatchedComponent(componentsMap, Component, displayName, React, options){ @@ -102,8 +102,8 @@ export default function whyDidYouRender(React, userOptions){ isShouldTrack = ( ( typeof componentNameOrComponent === 'function' || - componentNameOrComponent.$$typeof === REACT_MEMO_TYPE || - componentNameOrComponent.$$typeof === REACT_FORWARD_REF_TYPE + isMemoComponent(componentNameOrComponent) || + isForwardRefComponent(componentNameOrComponent) ) && shouldTrack(componentNameOrComponent, getDisplayName(componentNameOrComponent), options) )