From 59e5d5a39f8f4860c81219d3badc2a1726ac9a76 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 8 Feb 2019 19:37:17 +0000 Subject: [PATCH 1/3] Fix useImperativeHandle to have no deps by default --- .../react-reconciler/src/ReactFiberHooks.js | 4 +- ...eactHooksWithNoopRenderer-test.internal.js | 105 ++++++++++++++++++ 2 files changed, 107 insertions(+), 2 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 1443fb544cdd1..0a0654b698f8d 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -905,7 +905,7 @@ function mountImperativeHandle( // TODO: If deps are provided, should we skip comparing the ref itself? const effectDeps = - deps !== null && deps !== undefined ? deps.concat([ref]) : [ref]; + deps !== null && deps !== undefined ? deps.concat([ref]) : undefined; return mountEffectImpl( UpdateEffect, @@ -931,7 +931,7 @@ function updateImperativeHandle( // TODO: If deps are provided, should we skip comparing the ref itself? const effectDeps = - deps !== null && deps !== undefined ? deps.concat([ref]) : [ref]; + deps !== null && deps !== undefined ? deps.concat([ref]) : undefined; return updateEffectImpl( UpdateEffect, diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js index 446dd1bf7e78d..290651b117e77 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -1593,6 +1593,111 @@ describe('ReactHooksWithNoopRenderer', () => { }); }); + describe('useImperativeHandle', () => { + it('does not update when deps are the same', () => { + const INCREMENT = 'INCREMENT'; + + function reducer(state, action) { + return action === INCREMENT ? state + 1 : state; + } + + function Counter(props, ref) { + const [count, dispatch] = useReducer(reducer, 0); + useImperativeHandle(ref, () => ({count, dispatch}), []); + return ; + } + + Counter = forwardRef(Counter); + const counter = React.createRef(null); + ReactNoop.render(); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + expect(counter.current.count).toBe(0); + + act(() => { + counter.current.dispatch(INCREMENT); + }); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); + // Intentionally not updated because of [] deps: + expect(counter.current.count).toBe(0); + }); + + // Regression test for https://github.com/facebook/react/issues/14782 + it('automatically updates when deps are not specified', () => { + const INCREMENT = 'INCREMENT'; + + function reducer(state, action) { + return action === INCREMENT ? state + 1 : state; + } + + function Counter(props, ref) { + const [count, dispatch] = useReducer(reducer, 0); + useImperativeHandle(ref, () => ({count, dispatch})); + return ; + } + + Counter = forwardRef(Counter); + const counter = React.createRef(null); + ReactNoop.render(); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + expect(counter.current.count).toBe(0); + + act(() => { + counter.current.dispatch(INCREMENT); + }); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); + expect(counter.current.count).toBe(1); + }); + + it('updates when deps are different', () => { + const INCREMENT = 'INCREMENT'; + + function reducer(state, action) { + return action === INCREMENT ? state + 1 : state; + } + + let totalRefUpdates = 0; + function Counter(props, ref) { + const [count, dispatch] = useReducer(reducer, 0); + useImperativeHandle( + ref, + () => { + totalRefUpdates++; + return {count, dispatch}; + }, + [count], + ); + return ; + } + + Counter = forwardRef(Counter); + const counter = React.createRef(null); + ReactNoop.render(); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + expect(counter.current.count).toBe(0); + expect(totalRefUpdates).toBe(1); + + act(() => { + counter.current.dispatch(INCREMENT); + }); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); + expect(counter.current.count).toBe(1); + expect(totalRefUpdates).toBe(2); + + // Update that doesn't change the ref dependencies + ReactNoop.render(); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); + expect(counter.current.count).toBe(1); + expect(totalRefUpdates).toBe(2); // Should not increase since last time + }); + }); + describe('progressive enhancement (not supported)', () => { it('mount additional state', () => { let updateA; From 5406f00e1fc408bd50c2fcfe945ea9493d63a94e Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 8 Feb 2019 19:39:35 +0000 Subject: [PATCH 2/3] Save a byte? --- packages/react-reconciler/src/ReactFiberHooks.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 0a0654b698f8d..1ce3dace6617d 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -905,7 +905,7 @@ function mountImperativeHandle( // TODO: If deps are provided, should we skip comparing the ref itself? const effectDeps = - deps !== null && deps !== undefined ? deps.concat([ref]) : undefined; + deps !== null && deps !== undefined ? deps.concat([ref]) : deps; return mountEffectImpl( UpdateEffect, @@ -931,7 +931,7 @@ function updateImperativeHandle( // TODO: If deps are provided, should we skip comparing the ref itself? const effectDeps = - deps !== null && deps !== undefined ? deps.concat([ref]) : undefined; + deps !== null && deps !== undefined ? deps.concat([ref]) : deps; return updateEffectImpl( UpdateEffect, From 80c272d7c03c544f57eed82594e8d2ddced6baba Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 11 Feb 2019 18:35:01 +0000 Subject: [PATCH 3/3] Nit: null --- packages/react-reconciler/src/ReactFiberHooks.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 1ce3dace6617d..4ab6d7c7d7a59 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -905,7 +905,7 @@ function mountImperativeHandle( // TODO: If deps are provided, should we skip comparing the ref itself? const effectDeps = - deps !== null && deps !== undefined ? deps.concat([ref]) : deps; + deps !== null && deps !== undefined ? deps.concat([ref]) : null; return mountEffectImpl( UpdateEffect, @@ -931,7 +931,7 @@ function updateImperativeHandle( // TODO: If deps are provided, should we skip comparing the ref itself? const effectDeps = - deps !== null && deps !== undefined ? deps.concat([ref]) : deps; + deps !== null && deps !== undefined ? deps.concat([ref]) : null; return updateEffectImpl( UpdateEffect,