From fc5a5df74d608f6589e544c8aecd1b7daa79333c Mon Sep 17 00:00:00 2001 From: Jovi De Croock Date: Tue, 14 Jun 2022 16:36:10 +0200 Subject: [PATCH 1/3] cleanup repeated renders --- hooks/src/index.js | 39 ++++++++++--- hooks/src/internal.d.ts | 4 ++ hooks/test/browser/useEffect.test.js | 64 ++++++++++++++++++++++ hooks/test/browser/useLayoutEffect.test.js | 64 ++++++++++++++++++++++ hooks/test/browser/useMemo.test.js | 40 +++++++++++++- 5 files changed, 203 insertions(+), 8 deletions(-) diff --git a/hooks/src/index.js b/hooks/src/index.js index 4e1eded23c..cc6fa4f5b4 100644 --- a/hooks/src/index.js +++ b/hooks/src/index.js @@ -41,9 +41,20 @@ options._render = vnode => { hooks._pendingEffects = []; currentComponent._renderCallbacks = []; hooks._list.forEach(hookItem => { - if (hookItem._args) hookItem._args = undefined; + if (hookItem._pendingArgs) hookItem._pendingArgs = undefined; + if (hookItem._pendingValue) hookItem._pendingValue = undefined; }); } else { + hooks._list.forEach(hookItem => { + if (hookItem._pendingArgs) { + hookItem._args = hookItem._pendingArgs; + hookItem._pendingArgs = undefined; + } + if (hookItem._pendingValue) { + hookItem._value = hookItem._pendingValue; + hookItem._pendingValue = undefined; + } + }); hooks._pendingEffects.forEach(invokeCleanup); hooks._pendingEffects.forEach(invokeEffect); hooks._pendingEffects = []; @@ -66,6 +77,19 @@ options.diffed = vnode => { options._commit = (vnode, commitQueue) => { commitQueue.some(component => { try { + if (component.__hooks) { + component.__hooks._list.forEach(hookItem => { + if (hookItem._pendingArgs) { + hookItem._args = hookItem._pendingArgs; + hookItem._pendingArgs = undefined; + } + if (hookItem._pendingValue) { + hookItem._value = hookItem._pendingValue; + hookItem._pendingValue = undefined; + } + }); + } + component._renderCallbacks.forEach(invokeCleanup); component._renderCallbacks = component._renderCallbacks.filter(cb => cb._value ? invokeEffect(cb) : true @@ -174,8 +198,8 @@ export function useEffect(callback, args) { /** @type {import('./internal').EffectHookState} */ const state = getHookState(currentIndex++, 3); if (!options._skipEffects && argsChanged(state._args, args)) { - state._value = callback; - state._args = args; + state._pendingValue = callback; + state._pendingArgs = args; currentComponent.__hooks._pendingEffects.push(state); } @@ -189,8 +213,8 @@ export function useLayoutEffect(callback, args) { /** @type {import('./internal').EffectHookState} */ const state = getHookState(currentIndex++, 4); if (!options._skipEffects && argsChanged(state._args, args)) { - state._value = callback; - state._args = args; + state._pendingValue = callback; + state._pendingArgs = args; currentComponent._renderCallbacks.push(state); } @@ -230,9 +254,10 @@ export function useMemo(factory, args) { /** @type {import('./internal').MemoHookState} */ const state = getHookState(currentIndex++, 7); if (argsChanged(state._args, args)) { - state._value = factory(); - state._args = args; + state._pendingValue = factory(); + state._pendingArgs = args; state._factory = factory; + return state._pendingValue; } return state._value; diff --git a/hooks/src/internal.d.ts b/hooks/src/internal.d.ts index e6b51fb3aa..df4009c528 100644 --- a/hooks/src/internal.d.ts +++ b/hooks/src/internal.d.ts @@ -48,13 +48,17 @@ export type Cleanup = () => void; export interface EffectHookState { _value?: Effect; + _pendingValue?: any; _args?: any[]; + _pendingArgs?: any[]; _cleanup?: Cleanup | void; } export interface MemoHookState { _value?: any; + _pendingValue?: any; _args?: any[]; + _pendingArgs?: any[]; _factory?: () => any; } diff --git a/hooks/test/browser/useEffect.test.js b/hooks/test/browser/useEffect.test.js index 14771d9c7a..32be3a357f 100644 --- a/hooks/test/browser/useEffect.test.js +++ b/hooks/test/browser/useEffect.test.js @@ -529,4 +529,68 @@ describe('useEffect', () => { expect(calls.length).to.equal(1); expect(calls).to.deep.equal(['doing effecthi']); }); + + it('should not rerun committed effects', () => { + const calls = []; + const App = ({ i }) => { + const [greeting, setGreeting] = useState('hi'); + + useEffect(() => { + calls.push('doing effect' + greeting); + return () => { + calls.push('cleaning up' + greeting); + }; + }, []); + + if (i === 2) { + setGreeting('bye'); + } + + return

{greeting}

; + }; + + act(() => { + render(, scratch); + }); + expect(calls.length).to.equal(1); + expect(calls).to.deep.equal(['doing effecthi']); + + act(() => { + render(, scratch); + }); + }); + + it('should not schedule effects that have no change', () => { + const calls = []; + let set; + const App = ({ i }) => { + const [greeting, setGreeting] = useState('hi'); + set = setGreeting; + + useEffect(() => { + calls.push('doing effect' + greeting); + return () => { + calls.push('cleaning up' + greeting); + }; + }, [greeting]); + + if (greeting === 'bye') { + setGreeting('hi'); + } + + return

{greeting}

; + }; + + act(() => { + render(, scratch); + }); + expect(calls.length).to.equal(1); + expect(calls).to.deep.equal(['doing effecthi']); + + act(() => { + set('bye'); + }); + expect(calls.length).to.equal(1); + expect(calls).to.deep.equal(['doing effecthi']); + }); }); diff --git a/hooks/test/browser/useLayoutEffect.test.js b/hooks/test/browser/useLayoutEffect.test.js index a6c0df8dad..5a5e75eb7b 100644 --- a/hooks/test/browser/useLayoutEffect.test.js +++ b/hooks/test/browser/useLayoutEffect.test.js @@ -412,4 +412,68 @@ describe('useLayoutEffect', () => { expect(calls.length).to.equal(1); expect(calls).to.deep.equal(['doing effecthi']); }); + + it('should not rerun committed effects', () => { + const calls = []; + const App = ({ i }) => { + const [greeting, setGreeting] = useState('hi'); + + useLayoutEffect(() => { + calls.push('doing effect' + greeting); + return () => { + calls.push('cleaning up' + greeting); + }; + }, []); + + if (i === 2) { + setGreeting('bye'); + } + + return

{greeting}

; + }; + + act(() => { + render(, scratch); + }); + expect(calls.length).to.equal(1); + expect(calls).to.deep.equal(['doing effecthi']); + + act(() => { + render(, scratch); + }); + }); + + it('should not schedule effects that have no change', () => { + const calls = []; + let set; + const App = ({ i }) => { + const [greeting, setGreeting] = useState('hi'); + set = setGreeting; + + useLayoutEffect(() => { + calls.push('doing effect' + greeting); + return () => { + calls.push('cleaning up' + greeting); + }; + }, [greeting]); + + if (greeting === 'bye') { + setGreeting('hi'); + } + + return

{greeting}

; + }; + + act(() => { + render(, scratch); + }); + expect(calls.length).to.equal(1); + expect(calls).to.deep.equal(['doing effecthi']); + + act(() => { + set('bye'); + }); + expect(calls.length).to.equal(1); + expect(calls).to.deep.equal(['doing effecthi']); + }); }); diff --git a/hooks/test/browser/useMemo.test.js b/hooks/test/browser/useMemo.test.js index 68fb72e558..ebf4d46c6d 100644 --- a/hooks/test/browser/useMemo.test.js +++ b/hooks/test/browser/useMemo.test.js @@ -1,6 +1,7 @@ import { createElement, render } from 'preact'; import { setupScratch, teardown } from '../../../test/_util/helpers'; -import { useMemo } from 'preact/hooks'; +import { useMemo, useState } from 'preact/hooks'; +import { act } from 'preact/test-utils'; /** @jsx createElement */ @@ -122,4 +123,41 @@ describe('useMemo', () => { expect(spy2).to.be.calledThrice; expect(scratch.innerHTML).to.equal('
2

2

'); }); + + it('should not commit memoization from a skipped render', () => { + const calls = []; + let set; + const App = () => { + const [greeting, setGreeting] = useState('hi'); + set = setGreeting; + + const value = useMemo(() => { + calls.push('doing memo'); + return greeting; + }, [greeting]); + calls.push(`render ${value}`); + + if (greeting === 'bye') { + setGreeting('hi'); + } + + return

{value}

; + }; + + render(, scratch); + expect(calls.length).to.equal(2); + expect(calls).to.deep.equal(['doing memo', 'render hi']); + + act(() => { + set('bye'); + }); + expect(calls.length).to.equal(5); + expect(calls).to.deep.equal([ + 'doing memo', + 'render hi', + 'doing memo', + 'render bye', // We expect a missing "doing memo" here because we return to the previous args value + 'render hi' + ]); + }); }); From aaa219f6f2f4d3ff97d39f57fd123df93c1efce6 Mon Sep 17 00:00:00 2001 From: Jovi De Croock Date: Tue, 14 Jun 2022 16:41:48 +0200 Subject: [PATCH 2/3] avoid megamorhpic hook shapes --- hooks/src/index.js | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/hooks/src/index.js b/hooks/src/index.js index cc6fa4f5b4..37a0958fa8 100644 --- a/hooks/src/index.js +++ b/hooks/src/index.js @@ -41,19 +41,17 @@ options._render = vnode => { hooks._pendingEffects = []; currentComponent._renderCallbacks = []; hooks._list.forEach(hookItem => { - if (hookItem._pendingArgs) hookItem._pendingArgs = undefined; - if (hookItem._pendingValue) hookItem._pendingValue = undefined; + hookItem._pendingValue = hookItem._pendingArgs = undefined; }); } else { hooks._list.forEach(hookItem => { if (hookItem._pendingArgs) { hookItem._args = hookItem._pendingArgs; - hookItem._pendingArgs = undefined; } if (hookItem._pendingValue) { hookItem._value = hookItem._pendingValue; - hookItem._pendingValue = undefined; } + hookItem._pendingValue = hookItem._pendingArgs = undefined; }); hooks._pendingEffects.forEach(invokeCleanup); hooks._pendingEffects.forEach(invokeEffect); @@ -81,12 +79,11 @@ options._commit = (vnode, commitQueue) => { component.__hooks._list.forEach(hookItem => { if (hookItem._pendingArgs) { hookItem._args = hookItem._pendingArgs; - hookItem._pendingArgs = undefined; } if (hookItem._pendingValue) { hookItem._value = hookItem._pendingValue; - hookItem._pendingValue = undefined; } + hookItem._pendingValue = hookItem._pendingArgs = undefined; }); } From e13e9dbf2b74a885dc8eb3f109eb91ce19776512 Mon Sep 17 00:00:00 2001 From: Jovi De Croock Date: Tue, 14 Jun 2022 16:56:50 +0200 Subject: [PATCH 3/3] fixes --- hooks/src/index.js | 4 ++-- hooks/src/internal.d.ts | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/hooks/src/index.js b/hooks/src/index.js index 37a0958fa8..7aec24a73a 100644 --- a/hooks/src/index.js +++ b/hooks/src/index.js @@ -195,7 +195,7 @@ export function useEffect(callback, args) { /** @type {import('./internal').EffectHookState} */ const state = getHookState(currentIndex++, 3); if (!options._skipEffects && argsChanged(state._args, args)) { - state._pendingValue = callback; + state._value = callback; state._pendingArgs = args; currentComponent.__hooks._pendingEffects.push(state); @@ -210,7 +210,7 @@ export function useLayoutEffect(callback, args) { /** @type {import('./internal').EffectHookState} */ const state = getHookState(currentIndex++, 4); if (!options._skipEffects && argsChanged(state._args, args)) { - state._pendingValue = callback; + state._value = callback; state._pendingArgs = args; currentComponent._renderCallbacks.push(state); diff --git a/hooks/src/internal.d.ts b/hooks/src/internal.d.ts index df4009c528..a31ad578ce 100644 --- a/hooks/src/internal.d.ts +++ b/hooks/src/internal.d.ts @@ -48,7 +48,6 @@ export type Cleanup = () => void; export interface EffectHookState { _value?: Effect; - _pendingValue?: any; _args?: any[]; _pendingArgs?: any[]; _cleanup?: Cleanup | void;