From abb895b82c5720458bc5396c1f00fe1fe51a047f Mon Sep 17 00:00:00 2001 From: John Sloat Date: Mon, 1 Apr 2024 12:44:03 -0700 Subject: [PATCH] Ensure that an instantiated table can be rendered multiple times This fixes a pernicious issue where a table, after having been instantiated and rendered once, would not properly initialize all of its state. The most critical uninitiated element was the connected$ poller. The result of this bug was that any connected table would NOT UPDATE with connected stream updates after the first render. This issue is fixed by ensuring that on each first render of the table, the connected stream callback is registered. Previously it was only registered upon table instantiation. Also clarifies some logging & adds an option when fetching persisted data to suppress cache stream change trigger. --- src/UITable/TableClass.ts | 97 ++++++++++++++++++++++++--------------- src/io/persisted.ts | 7 ++- 2 files changed, 65 insertions(+), 39 deletions(-) diff --git a/src/UITable/TableClass.ts b/src/UITable/TableClass.ts index 7d104a0..5e1d161 100644 --- a/src/UITable/TableClass.ts +++ b/src/UITable/TableClass.ts @@ -2,6 +2,7 @@ import { ExcludeFalsy } from '../common'; import { Persisted } from '../io/persisted'; import RepeatingTimer from '../RepeatingTimer'; import { getIconPreloadHelpers } from '../sfSymbols'; +import { Stream } from '../streams'; import { AnyObj, NoParamFn } from '../types/utilTypes'; import { AfterFirstRender, @@ -21,26 +22,49 @@ import { } from './types'; import { catchTableError, reloadTableRows } from './utils'; -const getConnected$Poller = <$Data extends AnyObj>( - { $, ...timerOpts }: Connected$Opts<$Data>, - onUpdate: NoParamFn -) => { - let hasQueuedUpdate = false; - const callbackId = UUID.string(); - $.registerUpdateCallback({ - callback: () => (hasQueuedUpdate = true), - callbackId, - }); - const timer = new RepeatingTimer({ - ...timerOpts, - onFire: () => { - if (!hasQueuedUpdate) return; - hasQueuedUpdate = false; - onUpdate(); - }, - }); - return { timer, cleanup: () => $.unregisterUpdateCallback(callbackId) }; +type GetConnected$PollerOpts<$Data extends AnyObj> = { + connected$: Connected$Opts<$Data>; + onUpdate: NoParamFn; + tableName: string; }; +class Connected$Poller<$Data extends AnyObj> { + private connected$: Stream<$Data>; + private hasQueuedUpdate: boolean = false; + private timer: RepeatingTimer; + private callbackId: string; + + constructor({ + connected$, + onUpdate, + tableName, + }: GetConnected$PollerOpts<$Data>) { + const { $, ...timerOpts } = connected$; + this.connected$ = $ as Stream<$Data>; + this.callbackId = `Connected$ for table: ${tableName}`; + this.timer = new RepeatingTimer({ + ...timerOpts, + onFire: () => { + if (this.hasQueuedUpdate) { + this.hasQueuedUpdate = false; + onUpdate(); + } + }, + }); + } + + start() { + this.connected$.registerUpdateCallback({ + callbackId: this.callbackId, + callback: () => (this.hasQueuedUpdate = true), + }); + this.timer.start(); + } + + cleanup() { + this.timer.stop(); + this.connected$.unregisterUpdateCallback(this.callbackId); + } +} // @@ -77,14 +101,13 @@ class CallbackRegister { // export class Table { - private table: UITable; + private table = new UITable(); // Don't store connected$ props in this stream, since we will be duplicating // potentially huge amounts of data in the table. Rather, update the // `connected$ChangeCount` attribute with any stream change, thus triggering // changes for any callbacks/subscriptions to this stream. Use `getProps` to // access the combined props. private payload$: Payload$; - private connected$Poller?: RepeatingTimer; private name: string; private callbackID: string; private isActive = false; @@ -102,7 +125,7 @@ export class Table { props: boolean; runPrerenderCallbacks: boolean; }; - private renderCount: RenderCount; + private renderCount: RenderCount = 'NONE'; private defaultState?: State; private loadProps?: LoadProps; private getRows?: GetRows; @@ -118,7 +141,6 @@ export class Table { syncedPersistedState, connected$, }: TableOpts) { - this.table = new UITable(); this.table.showSeparators = showSeparators; this.name = name; this.fullscreen = fullscreen; @@ -132,26 +154,25 @@ export class Table { props: false, runPrerenderCallbacks: false, }; - this.renderCount = 'NONE'; this.callbackID = `Table: ${name}`; if (connected$) { - const connected$Poller = getConnected$Poller(connected$, () => { - const updated$Data = connected$.$.getData(); - this.payload$.updateData(({ connected$ChangeCount, ...rest }) => ({ - connected$ChangeCount: (connected$ChangeCount || 0) + 1, - ...rest, - })); - this.onConnected$Update?.(updated$Data as $Data); + const connected$Poller = new Connected$Poller({ + connected$, + tableName: name, + onUpdate: () => { + const updated$Data = connected$.$.getData(); + this.payload$.updateData(({ connected$ChangeCount, ...rest }) => ({ + connected$ChangeCount: (connected$ChangeCount || 0) + 1, + ...rest, + })); + this.onConnected$Update?.(updated$Data as $Data); + }, }); - this.connected$Poller = connected$Poller.timer; this.callbackRegister.set( 'connected$Poller', - () => this.connected$Poller!.start(), - () => { - this.connected$Poller!.stop(); - connected$Poller.cleanup(); - } + () => connected$Poller.start(), + () => connected$Poller.cleanup() ); } @@ -276,6 +297,8 @@ export class Table { this.isActive = false; await this.onDismiss?.(); this.callbackRegister.cleanupAll(); + this.has.runPrerenderCallbacks = false; + this.renderCount = 'NONE'; } // This function, on paper, does not always return `Promise`. However diff --git a/src/io/persisted.ts b/src/io/persisted.ts index 88b4352..d67cefe 100644 --- a/src/io/persisted.ts +++ b/src/io/persisted.ts @@ -93,7 +93,10 @@ type MaybePromiseWithoutPayload = { type GetData = MaybePromiseWithoutPayload; const getGetData = (ioObj: IOObject) => - (({ useCache = USE_CACHE_DEFAULT }: { useCache?: boolean } = {}): unknown => { + (({ + useCache = USE_CACHE_DEFAULT, + suppressChangeTrigger = false, + }: { useCache?: boolean; suppressChangeTrigger?: boolean } = {}): unknown => { const { cache$, defaultData } = ioObj; if (useCache) { if (!cache$) { @@ -107,7 +110,7 @@ const getGetData = (ioObj: IOObject) => _getPersistedJson(ioObj).then(persistedData => { const parsedData = persistedData ?? defaultData; if (cache$) { - cache$.setData({ data: parsedData }); + cache$.setData({ data: parsedData }, { suppressChangeTrigger }); } resolve(parsedData); });