Skip to content

Commit ba92d78

Browse files
ParkreinerParkreiner
committed
fix: resolve memoization wonkiness
1 parent 39d8641 commit ba92d78

File tree

10 files changed

+45
-37
lines changed

10 files changed

+45
-37
lines changed

‎site/src/components/SignInLayout/SignInLayout.tsx

+1-1
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import type { FC, PropsWithChildren } from "react";
44

55
export const SignInLayout: FC<PropsWithChildren> = ({ children }) => {
66
const year = useTimeSync({
7-
maxRefreshIntervalMs: Number.POSITIVE_INFINITY,
7+
idealRefreshIntervalMs: Number.POSITIVE_INFINITY,
88
select: (date) => date.getFullYear(),
99
});
1010

‎site/src/hooks/useTimeSync.tsx

+36-28
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ type TimeSyncInitOptions = Readonly<{
4949
/**
5050
* The function to use when clearing intervals.
5151
*
52-
* (i.e., Clearing a previous interval because the TimeSync needs to make a
52+
* (e.g., Clearing a previous interval because the TimeSync needs to make a
5353
* new interval to increase/decrease its update speed.)
5454
*/
5555
clearInterval: ClearInterval;
@@ -65,7 +65,7 @@ const defaultOptions: TimeSyncInitOptions = {
6565

6666
type SubscriptionEntry = Readonly<{
6767
id: string;
68-
maxRefreshIntervalMs: number;
68+
idealRefreshIntervalMs: number;
6969
onUpdate: (newDatetime: Date) => void;
7070
}>;
7171

@@ -136,15 +136,15 @@ export class TimeSync implements TimeSyncApi {
136136
}
137137

138138
const prevFastestInterval =
139-
this.#subscriptions[0]?.maxRefreshIntervalMs ?? Number.POSITIVE_INFINITY;
139+
this.#subscriptions[0]?.idealRefreshIntervalMs ?? Number.POSITIVE_INFINITY;
140140
if (this.#subscriptions.length > 1) {
141141
this.#subscriptions.sort(
142-
(e1, e2) => e1.maxRefreshIntervalMs - e2.maxRefreshIntervalMs,
142+
(e1, e2) => e1.idealRefreshIntervalMs - e2.idealRefreshIntervalMs,
143143
);
144144
}
145145

146146
const newFastestInterval =
147-
this.#subscriptions[0]?.maxRefreshIntervalMs ?? Number.POSITIVE_INFINITY;
147+
this.#subscriptions[0]?.idealRefreshIntervalMs ?? Number.POSITIVE_INFINITY;
148148
if (prevFastestInterval === newFastestInterval) {
149149
return;
150150
}
@@ -187,9 +187,9 @@ export class TimeSync implements TimeSyncApi {
187187
};
188188

189189
subscribe = (entry: SubscriptionEntry): (() => void) => {
190-
if (entry.maxRefreshIntervalMs <= 0) {
190+
if (entry.idealRefreshIntervalMs <= 0) {
191191
throw new Error(
192-
`Refresh interval ${entry.maxRefreshIntervalMs} must be a positive integer (or Infinity)`,
192+
`Refresh interval ${entry.idealRefreshIntervalMs} must be a positive integer (or Infinity)`,
193193
);
194194
}
195195

@@ -207,7 +207,7 @@ export class TimeSync implements TimeSyncApi {
207207
}
208208

209209
this.#subscriptions[subIndex] = entry;
210-
if (prev.maxRefreshIntervalMs !== entry.maxRefreshIntervalMs) {
210+
if (prev.idealRefreshIntervalMs !== entry.idealRefreshIntervalMs) {
211211
this.#reconcileRefreshIntervals();
212212
}
213213
return unsub;
@@ -246,7 +246,7 @@ export const TimeSyncProvider: FC<TimeSyncProviderProps> = ({
246246
// Making the TimeSync instance be initialized via React State, so that it's
247247
// easy to mock it out for component and story tests. TimeSync itself should
248248
// be treated like a pseudo-ref value, where its values can only be used in
249-
// very specific, React-approved ways (e.g., not directly in a render path)
249+
// very specific, React-approved ways
250250
const [readonlySync] = useState(
251251
() => new TimeSync(options ?? defaultOptions),
252252
);
@@ -259,14 +259,13 @@ export const TimeSyncProvider: FC<TimeSyncProviderProps> = ({
259259
};
260260

261261
type UseTimeSyncOptions<T = Date> = Readonly<{
262-
maxRefreshIntervalMs: number;
262+
idealRefreshIntervalMs: number;
263263

264264
/**
265265
* Allows you to transform any date values received from the TimeSync class.
266-
*
267-
* Note that select functions are not memoized and will run on every render
268-
* (similar to the ones in React Query and Redux Toolkit's default selectors).
269-
* Select functions should be kept cheap to recalculate.
266+
* Select functions work similarly to the selects from React Query and Redux
267+
* Toolkit – you don't need to memoize them, and they will only run when the
268+
* underlying TimeSync state has changed.
270269
*
271270
* Select functions must not be async. The hook will error out at the type
272271
* level if you provide one by mistake.
@@ -292,35 +291,44 @@ type ReactSubscriptionCallback = (notifyReact: () => void) => () => void;
292291
* interval.
293292
*/
294293
export function useTimeSync<T = Date>(options: UseTimeSyncOptions<T>): T {
295-
const { select, maxRefreshIntervalMs } = options;
294+
const { select, idealRefreshIntervalMs } = options;
296295

297296
// Abusing useId a little bit here. It's mainly meant to be used for
298297
// accessibility, but it also gives us a globally unique ID associated with
299298
// whichever component instance is consuming this hook
300299
const hookId = useId();
301300
const timeSync = useTimeSyncContext();
302301

303-
// We need to define this callback using useCallback instead of useEffectEvent
304-
// because we want the memoization to be invaliated when the refresh interval
305-
// changes. (When the subscription callback changes by reference, that causes
306-
// useSyncExternalStore to redo the subscription with the new callback). All
307-
// other values need to be included in the dependency array for correctness,
308-
// but their memory references should always be stable
302+
// We need to define this callback using useCallback instead of
303+
// useEffectEvent because we want the memoization to be invaliated when the
304+
// refresh interval changes. (When the subscription callback changes by
305+
// reference, that causes useSyncExternalStore to redo the subscription with
306+
// the new callback). All other values need to be included in the dependency
307+
// array for correctness, but their memory references should always be
308+
// stable
309309
const subscribe = useCallback<ReactSubscriptionCallback>(
310310
(notifyReact) => {
311311
return timeSync.subscribe({
312-
maxRefreshIntervalMs,
312+
idealRefreshIntervalMs,
313313
onUpdate: notifyReact,
314314
id: hookId,
315315
});
316316
},
317-
[timeSync, hookId, maxRefreshIntervalMs],
317+
[timeSync, hookId, idealRefreshIntervalMs],
318318
);
319319

320-
const currentTime = useSyncExternalStore(subscribe, () =>
321-
timeSync.getLatestDatetimeSnapshot(),
322-
);
320+
// Unlike subscribe, getNewState doesn't need to be memoized, because React
321+
// doesn't actually care about its memory reference. Whenever React gets
322+
// notified, it just calls the latest state function it received (whatever
323+
// it happens to be). React will only re-render if the value returned by the
324+
// function is different from what it got back last time (using === as the
325+
// comparison). This function ONLY gets called when the subscription is
326+
// first created, or when React gets notified via a subscription update.
327+
const getNewState = (): T => {
328+
const latestDate = timeSync.getLatestDatetimeSnapshot() as T & Date;
329+
const selected = (select?.(latestDate) ?? latestDate) as T;
330+
return selected;
331+
};
323332

324-
const recast = currentTime as T & Date;
325-
return select?.(recast) ?? recast;
333+
return useSyncExternalStore<T>(subscribe, getNewState);
326334
}

‎site/src/pages/CliInstallPage/CliInstallPageView.tsx

+1-1
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ type CliInstallPageViewProps = {
1111

1212
export const CliInstallPageView: FC<CliInstallPageViewProps> = ({ origin }) => {
1313
const year = useTimeSync({
14-
maxRefreshIntervalMs: Number.POSITIVE_INFINITY,
14+
idealRefreshIntervalMs: Number.POSITIVE_INFINITY,
1515
select: (date) => date.getFullYear(),
1616
});
1717

‎site/src/pages/DeploymentSettingsPage/LicensesSettingsPage/LicenseCard.tsx

+1-1
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ export const LicenseCard: FC<LicenseCardProps> = ({
2525
onRemove,
2626
isRemoving,
2727
}) => {
28-
const time = useTimeSync({ maxRefreshIntervalMs: 30_000 });
28+
const time = useTimeSync({ idealRefreshIntervalMs: 30_000 });
2929
const [licenseIDMarkedForRemoval, setLicenseIDMarkedForRemoval] = useState<
3030
number | undefined
3131
>(undefined);

‎site/src/pages/LoginPage/LoginPageView.tsx

+1-1
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ export const LoginPageView: FC<LoginPageViewProps> = ({
2929
redirectTo,
3030
}) => {
3131
const year = useTimeSync({
32-
maxRefreshIntervalMs: Number.POSITIVE_INFINITY,
32+
idealRefreshIntervalMs: Number.POSITIVE_INFINITY,
3333
select: (date) => date.getFullYear(),
3434
});
3535
const location = useLocation();

‎site/src/pages/TemplatePage/TemplateInsightsPage/DateRange.tsx

+1-1
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ interface DateRangeProps {
4343
}
4444

4545
export const DateRange: FC<DateRangeProps> = ({ value, onChange }) => {
46-
const currentTime = useTimeSync({ maxRefreshIntervalMs: 60_000 });
46+
const currentTime = useTimeSync({ idealRefreshIntervalMs: 60_000 });
4747
const selectionStatusRef = useRef<"idle" | "selecting">("idle");
4848
const [ranges, setRanges] = useState<RangesState>([
4949
{

‎site/src/pages/TemplatePage/TemplateInsightsPage/TemplateInsightsPage.tsx

+1-1
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ export default function TemplateInsightsPage() {
7171
const [searchParams, setSearchParams] = useSearchParams();
7272

7373
const insightsInterval = useTimeSync<InsightsInterval>({
74-
maxRefreshIntervalMs: MAX_REFRESH_ONE_DAY,
74+
idealRefreshIntervalMs: MAX_REFRESH_ONE_DAY,
7575
select: (newDatetime) => {
7676
const templateCreateDate = new Date(template.created_at);
7777
const hasFiveWeeksOrMore = addWeeks(templateCreateDate, 5) < newDatetime;

‎site/src/pages/WorkspacePage/AppStatuses.tsx

+1-1
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ export const AppStatuses: FC<AppStatusesProps> = ({
154154
referenceDate,
155155
}) => {
156156
const comparisonDate = useTimeSync({
157-
maxRefreshIntervalMs: MAX_REFRESH_ONE_MINUTE,
157+
idealRefreshIntervalMs: MAX_REFRESH_ONE_MINUTE,
158158
select: (dateState) => referenceDate ?? dateState,
159159
});
160160
const theme = useTheme();

‎site/src/pages/WorkspacePage/WorkspaceScheduleControls.tsx

+1-1
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,7 @@ const AutostopDisplay: FC<AutostopDisplayProps> = ({
163163
};
164164

165165
const activityStatus = useTimeSync({
166-
maxRefreshIntervalMs: 1_000,
166+
idealRefreshIntervalMs: 1_000,
167167
select: () => getWorkspaceActivityStatus(workspace),
168168
});
169169
const { message, tooltip, danger } = autostopDisplay(

‎site/src/pages/WorkspacesPage/LastUsed.tsx

+1-1
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ export const LastUsed: FC<LastUsedProps> = ({ lastUsedAt }) => {
1515
* @todo Verify that this is equivalent
1616
*/
1717
const [circle, message] = useTimeSync({
18-
maxRefreshIntervalMs: MAX_REFRESH_ONE_MINUTE,
18+
idealRefreshIntervalMs: MAX_REFRESH_ONE_MINUTE,
1919
select: (date) => {
2020
const t = dayjs(lastUsedAt);
2121
const deltaMsg = t.from(dayjs(date));

0 commit comments

Comments
 (0)