From 7eea5b9ef47a338e1f6bb945702f71b06703bb90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8rn=20A=2E=20Myrland?= Date: Thu, 15 Sep 2022 15:32:17 +0200 Subject: [PATCH] Optimize computed props (#764) * Optimize computed props An attempt to optimize computed props, by improving comparison of inputs & by checking if the value needs an update. The value comparison uses a naive comparison of of the JSON stringified values. This will support more complex inputs & avoid unneccessary updates for complex results (arrays, objects). related to #732 * refactor: Modifies computed properties to use fast-deep-equals * fix: Computed properties optimisations Co-authored-by: Sean Matheson --- .size-snapshot.json | 24 ++++++++++++------------ package.json | 1 + rollup.config.js | 1 + src/computed-properties.js | 34 +++++++++++++++++++++++++++++----- src/lib.js | 16 ---------------- tests/computed.test.js | 28 ++++++++++++++-------------- tests/dynamic-store.test.js | 4 +--- 7 files changed, 58 insertions(+), 50 deletions(-) diff --git a/.size-snapshot.json b/.size-snapshot.json index 945dc08db..5e9d414f5 100644 --- a/.size-snapshot.json +++ b/.size-snapshot.json @@ -1,27 +1,27 @@ { "index.js": { - "bundled": 51221, - "minified": 21486, - "gzipped": 6458, + "bundled": 51519, + "minified": 21541, + "gzipped": 6505, "treeshaked": { "rollup": { - "code": 135, - "import_statements": 131 + "code": 163, + "import_statements": 159 }, "webpack": { - "code": 2574 + "code": 2635 } } }, "index.cjs.js": { - "bundled": 52597, - "minified": 22671, - "gzipped": 6630 + "bundled": 52982, + "minified": 22785, + "gzipped": 6689 }, "index.iife.js": { - "bundled": 55510, - "minified": 18414, - "gzipped": 6030 + "bundled": 55887, + "minified": 18499, + "gzipped": 6074 }, "ie11.js": { "bundled": 102, diff --git a/package.json b/package.json index d5c93f1b1..63918fccf 100644 --- a/package.json +++ b/package.json @@ -50,6 +50,7 @@ }, "dependencies": { "@babel/runtime": "^7.17.2", + "fast-deep-equal": "^3.1.3", "immer": "^9.0.12", "redux": "^4.1.2", "redux-thunk": "^2.4.1", diff --git a/rollup.config.js b/rollup.config.js index f9e66ee7f..f26dcdee1 100644 --- a/rollup.config.js +++ b/rollup.config.js @@ -72,6 +72,7 @@ function createIIFEConfig(input, output, globalName) { redux: 'Redux', 'redux-thunk': 'reduxThunk', immer: 'immer', + equal: 'fast-deep-equal/es6', }, }, external, diff --git a/src/computed-properties.js b/src/computed-properties.js index c4d4df105..4d0fd0521 100644 --- a/src/computed-properties.js +++ b/src/computed-properties.js @@ -1,30 +1,54 @@ +import equal from 'fast-deep-equal/es6'; import { areInputsEqual } from './lib'; export function createComputedPropertyBinder(parentPath, key, def, _r) { let runOnce = false; let prevInputs = []; let prevValue; + let prevStoreState; + + let performingEqualityCheck = false; + + const areEqual = (a, b) => { + performingEqualityCheck = true; + const result = equal(a, b); + performingEqualityCheck = false; + return result; + }; + return function createComputedProperty(parentState, storeState) { Object.defineProperty(parentState, key, { configurable: true, enumerable: true, get: () => { + if (performingEqualityCheck) { + return prevValue; + } + const inputs = def.stateResolvers.map((resolver) => resolver(parentState, storeState), ); if ( runOnce && - (areInputsEqual(prevInputs, inputs) || + (storeState === prevStoreState || + areInputsEqual(inputs, prevInputs) || + // We don't want computed properties resolved every time an action + // is handled by the reducer. They need to remain lazy, only being + // computed when used by a component or getState call; (_r._i._cS.isInReducer && + // This is to account for strange errors that may occur via immer; new Error().stack.match(/shallowCopy/gi) !== null)) ) { - // We don't want computed properties resolved every time an action - // is handled by the reducer. They need to remain lazy, only being - // computed when used by a component or getState call. return prevValue; } + + const newValue = def.fn(...inputs); + if (!areEqual(newValue, prevValue)) { + prevValue = newValue; + } + prevInputs = inputs; - prevValue = def.fn(...inputs); + prevStoreState = storeState; runOnce = true; return prevValue; }, diff --git a/src/lib.js b/src/lib.js index be80ddd77..292e4d88c 100644 --- a/src/lib.js +++ b/src/lib.js @@ -175,22 +175,6 @@ export function areInputsEqual(newInputs, lastInputs) { return true; } -// export function memoizeOne(resultFn) { -// let lastArgs = []; -// let lastResult; -// let calledOnce = false; - -// return function memoized(...args) { -// if (calledOnce && areInputsEqual(args, lastArgs)) { -// return lastResult; -// } -// lastResult = resultFn(...args); -// calledOnce = true; -// lastArgs = args; -// return lastResult; -// }; -// } - export function useMemoOne( // getResult changes on every call, getResult, diff --git a/tests/computed.test.js b/tests/computed.test.js index dbf74e35c..4d18267a6 100644 --- a/tests/computed.test.js +++ b/tests/computed.test.js @@ -12,8 +12,7 @@ import { StoreProvider, } from '../src'; -/* -test.only('issue#633', () => { +test('issue#633', () => { // ARRANGE const createModel = (type) => ({ items: Array(100) @@ -33,9 +32,7 @@ test.only('issue#633', () => { }), removeCompletedItems: action((state) => { - console.log('foo'); const completedIds = state.completedItems.map((i) => i.id); - console.log(completedIds); state.items = state.items.filter( (item) => !completedIds.includes(item.id), @@ -48,24 +45,27 @@ test.only('issue#633', () => { def: createModel('def'), allCompletedItems: computed( [(_, storeState) => storeState.abc, (_, storeState) => storeState.def], - (abcItems, defItems) => { - console.log(abcItems.completedItems.length); - return [...abcItems.completedItems, ...defItems.completedItems]; - }, + (abcItems, defItems) => [ + ...abcItems.completedItems, + ...defItems.completedItems, + ], ), }); + // ASSERT + expect(store.getState().abc.completedItems.length).toBe(50); + expect(store.getState().allCompletedItems.length).toBe(100); + // ACT store.getActions().abc.removeCompletedItems(); store.getActions().def.removeCompletedItems(); // ASSERT expect(store.getState().abc.items.length).toBe(50); - expect(store.getState().abc.completedItems.length).toBe(50); + expect(store.getState().abc.completedItems.length).toBe(0); expect(store.getState().def.items.length).toBe(50); - expect(store.getState().allCompletedItems.length).toBe(100); + expect(store.getState().allCompletedItems.length).toBe(0); }); -*/ test('accessing computed properties within an action', () => { const store = createStore({ @@ -526,9 +526,9 @@ test('nested computed properties', () => { nested: { numbers: [1, 2, 3], - filteredNumbers: computed((state) => { - return state.numbers.filter((number) => number > 1); - }), + filteredNumbers: computed((state) => + state.numbers.filter((number) => number > 1), + ), }, // selectors diff --git a/tests/dynamic-store.test.js b/tests/dynamic-store.test.js index 5341cae5f..e4227c4d8 100644 --- a/tests/dynamic-store.test.js +++ b/tests/dynamic-store.test.js @@ -17,9 +17,7 @@ test('addModel', () => { push: action((state, payload) => { state.path = payload; }), - url: computed((state) => (name) => { - return `${state.path}${name}`; - }), + url: computed((state) => (name) => `${state.path}${name}`), }); // ASSERT