From 298b734539729dee42479b6dec67b6d5a4eb9d79 Mon Sep 17 00:00:00 2001 From: Joel Jeremy Marquez Date: Tue, 10 Dec 2024 09:35:23 -0800 Subject: [PATCH] Optimize useSheetValue (#3879) * Optimize useSheetValue * Fix lint * Fix mock bind * Reduce re-renders * Update useSheetValue * Update * Make QueryState immutable * Release notes --- .../components/spreadsheet/useSheetValue.ts | 40 +++++++++++------ .../src/client/SpreadsheetProvider.tsx | 14 +++--- packages/loot-core/src/mocks/spreadsheet.ts | 2 +- packages/loot-core/src/server/main.ts | 18 +------- packages/loot-core/src/shared/query.ts | 44 +++++++++++-------- upcoming-release-notes/3879.md | 6 +++ 6 files changed, 71 insertions(+), 53 deletions(-) create mode 100644 upcoming-release-notes/3879.md diff --git a/packages/desktop-client/src/components/spreadsheet/useSheetValue.ts b/packages/desktop-client/src/components/spreadsheet/useSheetValue.ts index ae0d09921ed..5ba10718a4e 100644 --- a/packages/desktop-client/src/components/spreadsheet/useSheetValue.ts +++ b/packages/desktop-client/src/components/spreadsheet/useSheetValue.ts @@ -1,4 +1,4 @@ -import { useState, useRef, useLayoutEffect } from 'react'; +import { useState, useRef, useLayoutEffect, useMemo } from 'react'; import { type Query } from 'loot-core/shared/query'; import { useSpreadsheet } from 'loot-core/src/client/SpreadsheetProvider'; @@ -30,15 +30,18 @@ export function useSheetValue< ): SheetValueResult['value'] { const { sheetName, fullSheetName } = useSheetName(binding); - const bindingObj = - typeof binding === 'string' - ? { name: binding, value: null, query: undefined } - : binding; + const bindingObj = useMemo( + () => + typeof binding === 'string' + ? { name: binding, value: null, query: undefined } + : binding, + [binding], + ); const spreadsheet = useSpreadsheet(); const [result, setResult] = useState>({ name: fullSheetName, - value: bindingObj.value === undefined ? null : bindingObj.value, + value: bindingObj.value ? bindingObj.value : null, query: bindingObj.query, }); const latestOnChange = useRef(onChange); @@ -48,15 +51,16 @@ export function useSheetValue< latestValue.current = result.value; useLayoutEffect(() => { - if (bindingObj.query) { - spreadsheet.createQuery(sheetName, bindingObj.name, bindingObj.query); - } + let isMounted = true; - return spreadsheet.bind( + const unbind = spreadsheet.bind( sheetName, - binding, - null, + bindingObj, (newResult: SheetValueResult) => { + if (!isMounted) { + return; + } + if (latestOnChange.current) { latestOnChange.current(newResult); } @@ -66,7 +70,17 @@ export function useSheetValue< } }, ); - }, [sheetName, bindingObj.name, JSON.stringify(bindingObj.query)]); + + return () => { + isMounted = false; + unbind(); + }; + }, [ + spreadsheet, + sheetName, + bindingObj.name, + bindingObj.query?.serializeAsString(), + ]); return result.value; } diff --git a/packages/loot-core/src/client/SpreadsheetProvider.tsx b/packages/loot-core/src/client/SpreadsheetProvider.tsx index 7c53ca515d4..6f8cfafbd78 100644 --- a/packages/loot-core/src/client/SpreadsheetProvider.tsx +++ b/packages/loot-core/src/client/SpreadsheetProvider.tsx @@ -62,22 +62,26 @@ function makeSpreadsheet() { }); } - bind(sheetName = '__global', binding, fields, cb) { + bind(sheetName = '__global', binding, callback) { binding = typeof binding === 'string' ? { name: binding, value: null } : binding; + if (binding.query) { + this.createQuery(sheetName, binding.name, binding.query); + } + const resolvedName = `${sheetName}!${binding.name}`; - const cleanup = this.observeCell(resolvedName, cb); + const cleanup = this.observeCell(resolvedName, callback); // Always synchronously call with the existing value if it has one. // This is a display optimization to avoid flicker. The LRU cache // will keep a number of recent nodes in memory. if (LRUValueCache.has(resolvedName)) { - cb(LRUValueCache.get(resolvedName)); + callback(LRUValueCache.get(resolvedName)); } if (cellCache[resolvedName] != null) { - cellCache[resolvedName].then(cb); + cellCache[resolvedName].then(callback); } else { const req = this.get(sheetName, binding.name); cellCache[resolvedName] = req; @@ -90,7 +94,7 @@ function makeSpreadsheet() { // with an old value depending on the order of messages) if (cellCache[resolvedName] === req) { LRUValueCache.set(resolvedName, result); - cb(result); + callback(result); } }); } diff --git a/packages/loot-core/src/mocks/spreadsheet.ts b/packages/loot-core/src/mocks/spreadsheet.ts index cdd02751c8a..87e4456a930 100644 --- a/packages/loot-core/src/mocks/spreadsheet.ts +++ b/packages/loot-core/src/mocks/spreadsheet.ts @@ -23,7 +23,7 @@ export function makeSpreadsheet() { this._getNode(sheetName, name).value = value; }, - bind(sheetName, binding, fields, cb) { + bind(sheetName, binding, cb) { const { name } = binding; const resolvedName = `${sheetName}!${name}`; if (!this.observers[resolvedName]) { diff --git a/packages/loot-core/src/server/main.ts b/packages/loot-core/src/server/main.ts index 24fb626f487..89a29484f7a 100644 --- a/packages/loot-core/src/server/main.ts +++ b/packages/loot-core/src/server/main.ts @@ -516,22 +516,8 @@ handlers['make-filters-from-conditions'] = async function ({ conditions }) { }; handlers['getCell'] = async function ({ sheetName, name }) { - // Fields is no longer used - hardcode - const fields = ['name', 'value']; const node = sheet.get()._getNode(resolveName(sheetName, name)); - if (fields) { - const res = {}; - fields.forEach(field => { - if (field === 'run') { - res[field] = node._run ? node._run.toString() : null; - } else { - res[field] = node[field]; - } - }); - return res; - } else { - return node; - } + return { name: node.name, value: node.value }; }; handlers['getCells'] = async function ({ names }) { @@ -1111,7 +1097,7 @@ handlers['accounts-bank-sync'] = async function ({ ids = [] }) { const accounts = await db.runQuery( ` - SELECT a.*, b.bank_id as bankId + SELECT a.*, b.bank_id as bankId FROM accounts a LEFT JOIN banks b ON a.bank = b.id WHERE a.tombstone = 0 AND a.closed = 0 diff --git a/packages/loot-core/src/shared/query.ts b/packages/loot-core/src/shared/query.ts index 4b38e62ac8c..b53ad149867 100644 --- a/packages/loot-core/src/shared/query.ts +++ b/packages/loot-core/src/shared/query.ts @@ -5,18 +5,18 @@ type ObjectExpression = { }; export type QueryState = { - table: string; - tableOptions: Record; - filterExpressions: Array; - selectExpressions: Array; - groupExpressions: Array; - orderExpressions: Array; - calculation: boolean; - rawMode: boolean; - withDead: boolean; - validateRefs: boolean; - limit: number | null; - offset: number | null; + get table(): string; + get tableOptions(): Readonly>; + get filterExpressions(): ReadonlyArray; + get selectExpressions(): ReadonlyArray; + get groupExpressions(): ReadonlyArray; + get orderExpressions(): ReadonlyArray; + get calculation(): boolean; + get rawMode(): boolean; + get withDead(): boolean; + get validateRefs(): boolean; + get limit(): number | null; + get offset(): number | null; }; export class Query { @@ -76,15 +76,19 @@ export class Query { exprs = [exprs]; } - const query = new Query({ ...this.state, selectExpressions: exprs }); - query.state.calculation = false; - return query; + return new Query({ + ...this.state, + selectExpressions: exprs, + calculation: false, + }); } calculate(expr: ObjectExpression | string) { - const query = this.select({ result: expr }); - query.state.calculation = true; - return query; + return new Query({ + ...this.state, + selectExpressions: [{ result: expr }], + calculation: true, + }); } groupBy(exprs: ObjectExpression | string | Array) { @@ -140,6 +144,10 @@ export class Query { serialize() { return this.state; } + + serializeAsString() { + return JSON.stringify(this.serialize()); + } } export function getPrimaryOrderBy( diff --git a/upcoming-release-notes/3879.md b/upcoming-release-notes/3879.md new file mode 100644 index 00000000000..89887d8dedf --- /dev/null +++ b/upcoming-release-notes/3879.md @@ -0,0 +1,6 @@ +--- +category: Maintenance +authors: [joel-jeremy] +--- + +Optimize useSheetValue hook