From a68ca9a5b54394bb61131f73f38329bfaeae331c Mon Sep 17 00:00:00 2001 From: Sophie Alpert Date: Thu, 11 Oct 2018 13:04:42 -0700 Subject: [PATCH] React.pure automatically forwards ref (#13822) We're not planning to encourage legacy context, and without this change, it's difficult to use pure+forwardRef together. We could special-case `pure(forwardRef(...))` but this is hopefully simpler. ```js React.pure(function(props, ref) { // ... }); ``` --- .../src/ReactFiberBeginWork.js | 7 +- .../src/__tests__/ReactPure-test.internal.js | 255 +++++++++++------- 2 files changed, 161 insertions(+), 101 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index d5b3545622862..c6bcf94324067 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -236,6 +236,7 @@ function updatePureComponent( renderExpirationTime: ExpirationTime, ) { const render = Component.render; + const ref = workInProgress.ref; if ( current !== null && @@ -246,7 +247,7 @@ function updatePureComponent( // Default to shallow comparison let compare = Component.compare; compare = compare !== null ? compare : shallowEqual; - if (compare(prevProps, nextProps)) { + if (workInProgress.ref === current.ref && compare(prevProps, nextProps)) { return bailoutOnAlreadyFinishedWork( current, workInProgress, @@ -261,10 +262,10 @@ function updatePureComponent( if (__DEV__) { ReactCurrentOwner.current = workInProgress; ReactCurrentFiber.setCurrentPhase('render'); - nextChildren = render(nextProps); + nextChildren = render(nextProps, ref); ReactCurrentFiber.setCurrentPhase(null); } else { - nextChildren = render(nextProps); + nextChildren = render(nextProps, ref); } // React DevTools reads this flag. diff --git a/packages/react-reconciler/src/__tests__/ReactPure-test.internal.js b/packages/react-reconciler/src/__tests__/ReactPure-test.internal.js index d5ea81a2f5e34..3d53293599919 100644 --- a/packages/react-reconciler/src/__tests__/ReactPure-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactPure-test.internal.js @@ -35,14 +35,14 @@ describe('pure', () => { } // Tests should run against both the lazy and non-lazy versions of `pure`. - // To make the tests work for both versions, we wrap the non-lazy verion in + // To make the tests work for both versions, we wrap the non-lazy version in // a lazy function component. sharedTests('normal', (...args) => { const Pure = React.pure(...args); - function Indirection(props) { - return ; + function Indirection(props, ref) { + return ; } - return Promise.resolve(Indirection); + return Promise.resolve(React.forwardRef(Indirection)); }); sharedTests('lazy', (...args) => Promise.resolve(React.pure(...args))); @@ -84,110 +84,169 @@ describe('pure', () => { expect(ReactNoop.flush()).toEqual([1]); expect(ReactNoop.getChildren()).toEqual([span(1)]); }); - }); - it("does not bail out if there's a context change", async () => { - const {unstable_Suspense: Suspense} = React; - - const CountContext = React.createContext(0); - - function Counter(props) { - const count = CountContext.unstable_read(); - return ; - } - Counter = pure(Counter); - - class Parent extends React.Component { - state = {count: 0}; - render() { - return ( - - - - - + it("does not bail out if there's a context change", async () => { + const {unstable_Suspense: Suspense} = React; + + const CountContext = React.createContext(0); + + function Counter(props) { + const count = CountContext.unstable_read(); + return ; + } + Counter = pure(Counter); + + class Parent extends React.Component { + state = {count: 0}; + render() { + return ( + + + + + + ); + } + } + + const parent = React.createRef(null); + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual([]); + await Promise.resolve(); + expect(ReactNoop.flush()).toEqual(['Count: 0']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + + // Should bail out because props have not changed + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual([]); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + + // Should update because there was a context change + parent.current.setState({count: 1}); + expect(ReactNoop.flush()).toEqual(['Count: 1']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); + }); + + it('accepts custom comparison function', async () => { + const {unstable_Suspense: Suspense} = React; + + function Counter({count}) { + return ; + } + Counter = pure(Counter, (oldProps, newProps) => { + ReactNoop.yield( + `Old count: ${oldProps.count}, New count: ${newProps.count}`, ); + return oldProps.count === newProps.count; + }); + + ReactNoop.render( + + + , + ); + expect(ReactNoop.flush()).toEqual([]); + await Promise.resolve(); + expect(ReactNoop.flush()).toEqual([0]); + expect(ReactNoop.getChildren()).toEqual([span(0)]); + + // Should bail out because props have not changed + ReactNoop.render( + + + , + ); + expect(ReactNoop.flush()).toEqual(['Old count: 0, New count: 0']); + expect(ReactNoop.getChildren()).toEqual([span(0)]); + + // Should update because count prop changed + ReactNoop.render( + + + , + ); + expect(ReactNoop.flush()).toEqual(['Old count: 0, New count: 1', 1]); + expect(ReactNoop.getChildren()).toEqual([span(1)]); + }); + + it('warns for class components', () => { + class SomeClass extends React.Component { + render() { + return null; + } } - } - - const parent = React.createRef(null); - ReactNoop.render(); - expect(ReactNoop.flush()).toEqual([]); - await Promise.resolve(); - expect(ReactNoop.flush()).toEqual(['Count: 0']); - expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); - - // Should bail out because props have not changed - ReactNoop.render(); - expect(ReactNoop.flush()).toEqual([]); - expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); - - // Should update because there was a context change - parent.current.setState({count: 1}); - expect(ReactNoop.flush()).toEqual(['Count: 1']); - expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); - }); + expect(() => pure(SomeClass)).toWarnDev( + 'pure: The first argument must be a function component.', + {withoutStack: true}, + ); + }); - it('accepts custom comparison function', async () => { - const {unstable_Suspense: Suspense} = React; + it('warns if first argument is not a function', () => { + expect(() => pure()).toWarnDev( + 'pure: The first argument must be a function component. Instead ' + + 'received: undefined', + {withoutStack: true}, + ); + }); - function Counter({count}) { - return ; - } - Counter = pure(Counter, (oldProps, newProps) => { - ReactNoop.yield( - `Old count: ${oldProps.count}, New count: ${newProps.count}`, + it('forwards ref', async () => { + const {unstable_Suspense: Suspense} = React; + const Transparent = pure((props, ref) => { + return
; + }); + const divRef = React.createRef(); + + ReactNoop.render( + + + , ); - return oldProps.count === newProps.count; + ReactNoop.flush(); + await Promise.resolve(); + ReactNoop.flush(); + expect(divRef.current.type).toBe('div'); }); - ReactNoop.render( - - - , - ); - expect(ReactNoop.flush()).toEqual([]); - await Promise.resolve(); - expect(ReactNoop.flush()).toEqual([0]); - expect(ReactNoop.getChildren()).toEqual([span(0)]); - - // Should bail out because props have not changed - ReactNoop.render( - - - , - ); - expect(ReactNoop.flush()).toEqual(['Old count: 0, New count: 0']); - expect(ReactNoop.getChildren()).toEqual([span(0)]); - - // Should update because count prop changed - ReactNoop.render( - - - , - ); - expect(ReactNoop.flush()).toEqual(['Old count: 0, New count: 1', 1]); - expect(ReactNoop.getChildren()).toEqual([span(1)]); - }); + it('updates if only ref changes', async () => { + const {unstable_Suspense: Suspense} = React; + const Transparent = pure((props, ref) => { + return [,
]; + }); - it('warns for class components', () => { - class SomeClass extends React.Component { - render() { - return null; - } - } - expect(() => pure(SomeClass)).toWarnDev( - 'pure: The first argument must be a function component.', - {withoutStack: true}, - ); - }); + const divRef = React.createRef(); + const divRef2 = React.createRef(); + + ReactNoop.render( + + + , + ); + expect(ReactNoop.flush()).toEqual([]); + await Promise.resolve(); + expect(ReactNoop.flush()).toEqual(['Text']); + expect(divRef.current.type).toBe('div'); + expect(divRef2.current).toBe(null); + + // Should re-render (new ref) + ReactNoop.render( + + + , + ); + expect(ReactNoop.flush()).toEqual(['Text']); + expect(divRef.current).toBe(null); + expect(divRef2.current.type).toBe('div'); - it('warns if first argument is not a function', () => { - expect(() => pure()).toWarnDev( - 'pure: The first argument must be a function component. Instead ' + - 'received: undefined', - {withoutStack: true}, - ); + // Should not re-render (same ref) + ReactNoop.render( + + + , + ); + expect(ReactNoop.flush()).toEqual([]); + expect(divRef.current).toBe(null); + expect(divRef2.current.type).toBe('div'); + }); }); } });