From 0f8cd9a2462d52947a34c8a6bc1b9402eea70fc6 Mon Sep 17 00:00:00 2001
From: Parkreiner <michaelsmith@coder.com>
Date: Tue, 3 Oct 2023 12:04:12 +0000
Subject: [PATCH 1/5] chore: add image pre-loading file and scaffolding for
test
---
site/src/hooks/images.test.ts | 77 ++++++++++++++++
site/src/hooks/images.ts | 162 ++++++++++++++++++++++++++++++++++
2 files changed, 239 insertions(+)
create mode 100644 site/src/hooks/images.test.ts
create mode 100644 site/src/hooks/images.ts
diff --git a/site/src/hooks/images.test.ts b/site/src/hooks/images.test.ts
new file mode 100644
index 0000000000000..88c4486d38cb9
--- /dev/null
+++ b/site/src/hooks/images.test.ts
@@ -0,0 +1,77 @@
+/**
+ * Work in progress. Mainly because I need to figure out how best to deal with
+ * a global constructor that implicitly makes HTTP requests in the background.
+ */
+import { useImagePreloading, useThrottledImageLoader } from "./images";
+
+/**
+ * Probably not on the right track with this one. Probably need to redo.
+ */
+class MockImage {
+ #unusedWidth = 0;
+ #unusedHeight = 0;
+ #src = "";
+ completed = false;
+
+ constructor(width?: number, height?: number) {
+ this.#unusedWidth = width ?? 0;
+ this.#unusedHeight = height ?? 0;
+ }
+
+ get src() {
+ return this.#src;
+ }
+
+ set src(newSrc: string) {
+ this.#src = newSrc;
+ }
+}
+
+beforeAll(() => {
+ jest.useFakeTimers();
+ jest.spyOn(global, "Image").mockImplementation(MockImage);
+});
+
+test(`${useImagePreloading.name}`, () => {
+ it.skip("Should passively preload images after a render", () => {
+ expect.hasAssertions();
+ });
+
+ it.skip("Should kick off a new pre-load if the content of the images changes after a re-render", () => {
+ expect.hasAssertions();
+ });
+
+ it.skip("Should not kick off a new pre-load if the array changes by reference, but the content is the same", () => {
+ expect.hasAssertions();
+ });
+});
+
+test(`${useThrottledImageLoader.name}`, () => {
+ it.skip("Should pre-load all images passed in the first time the function is called, no matter what", () => {
+ expect.hasAssertions();
+ });
+
+ it.skip("Should throttle all calls to the function made within the specified throttle time", () => {
+ expect.hasAssertions();
+ });
+
+ it.skip("Should always return a cleanup function", () => {
+ expect.hasAssertions();
+ });
+
+ it.skip("Should reset its own state if the returned-out cleanup function is called", () => {
+ expect.hasAssertions();
+ });
+
+ it.skip("Should not trigger the throttle if the images argument is undefined", () => {
+ expect.hasAssertions();
+ });
+
+ it.skip("Should support arbitrary throttle values", () => {
+ expect.hasAssertions();
+ });
+
+ it.skip("Should reset all of its state if the throttle value passed into the hook changes in a re-render", () => {
+ expect.hasAssertions();
+ });
+});
diff --git a/site/src/hooks/images.ts b/site/src/hooks/images.ts
new file mode 100644
index 0000000000000..0e6cbc11fd74f
--- /dev/null
+++ b/site/src/hooks/images.ts
@@ -0,0 +1,162 @@
+/**
+ * @file Defines general-purpose hooks/utilities for working with images.
+ *
+ * Mainly for pre-loading images to minimize periods when a UI renders with data
+ * but takes a few extra seconds for the images to load in.
+ */
+import { useCallback, useEffect, useRef, useState } from "react";
+
+const MAX_RETRIES = 3;
+
+/**
+ * Tracks the loading status of an individual image.
+ */
+type ImageTrackerEntry = {
+ image: HTMLImageElement;
+ status: "loading" | "error" | "success";
+ retries: number;
+};
+
+/**
+ * Tracks all images pre-loaded by hooks/utilities defined in this file. This
+ * is a single source of shared mutable state.
+ */
+const imageTracker = new Map<string, ImageTrackerEntry>();
+
+/**
+ * General pre-load utility used throughout the file. Returns a cleanup function
+ * to help work with React.useEffect.
+ *
+ * Currently treated as an implementation detail, so it's not exported, but it
+ * might make sense to break it out into a separate file down the line.
+ */
+function preloadImages(imageUrls?: readonly string[]): () => void {
+ if (imageUrls === undefined) {
+ // Just a noop
+ return () => {};
+ }
+
+ const retryTimeoutIds: number[] = [];
+
+ for (const imgUrl of imageUrls) {
+ const prevEntry = imageTracker.get(imgUrl);
+
+ if (prevEntry === undefined) {
+ const dummyImage = new Image();
+ dummyImage.src = imgUrl;
+
+ const entry: ImageTrackerEntry = {
+ image: dummyImage,
+ status: "loading",
+ retries: 0,
+ };
+
+ dummyImage.onload = () => {
+ entry.status = "success";
+ };
+
+ dummyImage.onerror = () => {
+ if (imgUrl !== "") {
+ entry.status = "error";
+ }
+ };
+
+ imageTracker.set(imgUrl, entry);
+ continue;
+ }
+
+ const skipRetry =
+ prevEntry.status === "loading" ||
+ prevEntry.status === "success" ||
+ prevEntry.retries === MAX_RETRIES;
+
+ if (skipRetry) {
+ continue;
+ }
+
+ prevEntry.image.src = "";
+ const retryId = window.setTimeout(() => {
+ prevEntry.image.src = imgUrl;
+ prevEntry.retries++;
+ }, 0);
+
+ retryTimeoutIds.push(retryId);
+ }
+
+ return () => {
+ for (const id of retryTimeoutIds) {
+ window.clearTimeout(id);
+ }
+ };
+}
+
+/**
+ * Exposes a throttled version of preloadImages. Useful for tying pre-loads to
+ * things like mouse hovering.
+ *
+ * The throttling state is always associated with the component instance,
+ * meaning that one component being throttled won't prevent other components
+ * from making requests.
+ */
+export function useThrottledImageLoader(throttleTimeMs = 500) {
+ const throttledRef = useRef(false);
+ const loadedCleanupRef = useRef<(() => void) | null>(null);
+
+ useEffect(() => {
+ loadedCleanupRef.current?.();
+ }, [throttleTimeMs]);
+
+ return useCallback(
+ (imgUrls?: readonly string[]) => {
+ if (throttledRef.current || imgUrls === undefined) {
+ // Noop
+ return () => {};
+ }
+
+ throttledRef.current = true;
+ const cleanup = preloadImages(imgUrls);
+ loadedCleanupRef.current = cleanup;
+
+ const timeoutId = window.setTimeout(() => {
+ throttledRef.current = false;
+ }, throttleTimeMs);
+
+ return () => {
+ cleanup();
+ loadedCleanupRef.current = null;
+
+ window.clearTimeout(timeoutId);
+ throttledRef.current = false;
+ };
+ },
+ [throttleTimeMs],
+ );
+}
+
+/**
+ * Sets up passive image-preloading for a component.
+ *
+ * Has logic in place to minimize the risks of an array being passed in, even
+ * if the array's memory reference changes every render.
+ */
+export function useImagePreloading(imgUrls?: readonly string[]) {
+ // Doing weird, hacky nonsense to guarantee useEffect doesn't run too often,
+ // even if consuming component doesn't stabilize value of imgUrls
+ const [cachedUrls, setCachedUrls] = useState(imgUrls);
+
+ // Very uncommon pattern, but it's based on something from the official React
+ // docs, and the comparison should have no perceivable effect on performance
+ if (cachedUrls !== imgUrls) {
+ const changedByValue =
+ imgUrls?.length !== cachedUrls?.length ||
+ !cachedUrls?.every((url, index) => url === imgUrls?.[index]);
+
+ if (changedByValue) {
+ setCachedUrls(imgUrls);
+ }
+ }
+
+ useEffect(() => {
+ return preloadImages(cachedUrls);
+ }, [cachedUrls]);
+}
From cf824af4351bf0b869c4eaf055c4e18a6ad74641 Mon Sep 17 00:00:00 2001
From: Parkreiner <michaelsmith@coder.com>
Date: Wed, 4 Oct 2023 15:59:32 +0000
Subject: [PATCH 2/5] docs: beef up docs for weird behavior
---
site/src/hooks/images.ts | 41 ++++++++++++++++++++++++++++++----------
1 file changed, 31 insertions(+), 10 deletions(-)
diff --git a/site/src/hooks/images.ts b/site/src/hooks/images.ts
index 0e6cbc11fd74f..8ff0b4306631f 100644
--- a/site/src/hooks/images.ts
+++ b/site/src/hooks/images.ts
@@ -144,16 +144,37 @@ export function useImagePreloading(imgUrls?: readonly string[]) {
// even if consuming component doesn't stabilize value of imgUrls
const [cachedUrls, setCachedUrls] = useState(imgUrls);
- // Very uncommon pattern, but it's based on something from the official React
- // docs, and the comparison should have no perceivable effect on performance
- if (cachedUrls !== imgUrls) {
- const changedByValue =
- imgUrls?.length !== cachedUrls?.length ||
- !cachedUrls?.every((url, index) => url === imgUrls?.[index]);
-
- if (changedByValue) {
- setCachedUrls(imgUrls);
- }
+ // For performance reasons, this array comparison only goes one level deep
+ const changedByValue =
+ cachedUrls !== imgUrls &&
+ (imgUrls?.length !== cachedUrls?.length ||
+ !cachedUrls?.every((url, index) => url === imgUrls?.[index]));
+
+ /**
+ * If this state dispatch were called inside useEffect, that would mean that
+ * the component and all of its children would render in full and get painted
+ * to the screen, the effect would fire, and then the component and all of its
+ * children would need to re-render and re-paint.
+ *
+ * Not a big deal for small components; possible concern when this hook is
+ * designed to be used anywhere and could be used at the top of the app.
+ *
+ * Calling the state dispatch inline means that the React invalidates the
+ * current component's output immediately. So the current render finishes,
+ * React flushes the state changes, throws away the render result, and
+ * immediately redoes the render with the new state. This happens before any
+ * painting happens, and before React even tries to touch the children.
+ *
+ * Basically, this pattern is weird and ugly, but it guarantees that no matter
+ * how complicated a component is, the cost of updating the state is always
+ * limited to one component total, and never any of its descendants. And the
+ * cost is limited to redoing an internal React render, not a render plus a
+ * set of DOM updates.
+ *
+ * @see {@link https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes}
+ */
+ if (changedByValue) {
+ setCachedUrls(imgUrls);
}
useEffect(() => {
From 9ed0dc3cd625f6fb7522b15194ddc50be0593ac0 Mon Sep 17 00:00:00 2001
From: Parkreiner <michaelsmith@coder.com>
Date: Wed, 4 Oct 2023 16:41:53 +0000
Subject: [PATCH 3/5] fix: update logic issue with cleanup functions
---
site/src/hooks/images.ts | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/site/src/hooks/images.ts b/site/src/hooks/images.ts
index 8ff0b4306631f..0d329eb125305 100644
--- a/site/src/hooks/images.ts
+++ b/site/src/hooks/images.ts
@@ -99,11 +99,15 @@ function preloadImages(imageUrls?: readonly string[]): () => void {
* from making requests.
*/
export function useThrottledImageLoader(throttleTimeMs = 500) {
+ /**
+ * It's possible to reduce the amount of state by only having the cleanup ref;
+ * tried it, but it made the code a lot harder to read and reason about
+ */
const throttledRef = useRef(false);
- const loadedCleanupRef = useRef<(() => void) | null>(null);
+ const componentCleanupRef = useRef<(() => void) | null>(null);
useEffect(() => {
- loadedCleanupRef.current?.();
+ componentCleanupRef.current?.();
}, [throttleTimeMs]);
return useCallback(
@@ -114,20 +118,22 @@ export function useThrottledImageLoader(throttleTimeMs = 500) {
}
throttledRef.current = true;
- const cleanup = preloadImages(imgUrls);
- loadedCleanupRef.current = cleanup;
+ const imagesCleanup = preloadImages(imgUrls);
const timeoutId = window.setTimeout(() => {
throttledRef.current = false;
}, throttleTimeMs);
- return () => {
- cleanup();
- loadedCleanupRef.current = null;
-
+ const componentCleanup = () => {
+ imagesCleanup();
window.clearTimeout(timeoutId);
+
throttledRef.current = false;
+ componentCleanupRef.current = null;
};
+
+ componentCleanupRef.current = componentCleanup;
+ return componentCleanup;
},
[throttleTimeMs],
);
From 2c987ba4be50bc482f8cbcd354181978b7b434f0 Mon Sep 17 00:00:00 2001
From: Parkreiner <michaelsmith@coder.com>
Date: Wed, 4 Oct 2023 16:42:06 +0000
Subject: [PATCH 4/5] wip: commit progress for tests
---
site/src/hooks/images.test.ts | 53 ++++++++++++++++++++++++++---------
1 file changed, 39 insertions(+), 14 deletions(-)
diff --git a/site/src/hooks/images.test.ts b/site/src/hooks/images.test.ts
index 88c4486d38cb9..311992a757038 100644
--- a/site/src/hooks/images.test.ts
+++ b/site/src/hooks/images.test.ts
@@ -1,22 +1,18 @@
-/**
- * Work in progress. Mainly because I need to figure out how best to deal with
- * a global constructor that implicitly makes HTTP requests in the background.
- */
import { useImagePreloading, useThrottledImageLoader } from "./images";
/**
- * Probably not on the right track with this one. Probably need to redo.
+ * This is weird and clunky, and the mocking seems to have more edge cases than
+ * the code I'm trying to test.
+ *
+ * HTMLImageElement doesn't go through fetch, but still it might be worth trying
+ * to integrate this with MSW
*/
+let mode: "alwaysSucceed" | "alwaysFail" = "alwaysSucceed";
+
class MockImage {
- #unusedWidth = 0;
- #unusedHeight = 0;
#src = "";
- completed = false;
-
- constructor(width?: number, height?: number) {
- this.#unusedWidth = width ?? 0;
- this.#unusedHeight = height ?? 0;
- }
+ onload: (() => void) | undefined = undefined;
+ onerror: (() => void) | undefined = undefined;
get src() {
return this.#src;
@@ -24,12 +20,41 @@ class MockImage {
set src(newSrc: string) {
this.#src = newSrc;
+ this.#simulateHttpRequest(newSrc);
+ }
+
+ #simulateHttpRequest(src: string) {
+ const promise = new Promise<void>((resolve, reject) => {
+ if (src === "") {
+ reject();
+ }
+
+ const settlePromise = mode === "alwaysSucceed" ? resolve : reject;
+ setTimeout(settlePromise, 100);
+ });
+
+ // Need arrow functions because onload/onerror are allowed to mutate in the
+ // original HTMLImageElement
+ void promise.then(() => this.onload?.());
+ void promise.catch(() => this.onerror?.());
}
}
beforeAll(() => {
jest.useFakeTimers();
- jest.spyOn(global, "Image").mockImplementation(MockImage);
+
+ jest.spyOn(global, "Image").mockImplementation(() => {
+ return new MockImage() as unknown as HTMLImageElement;
+ });
+});
+
+beforeEach(() => {
+ mode = "alwaysSucceed";
+});
+
+afterAll(() => {
+ jest.useRealTimers();
+ jest.clearAllMocks();
});
test(`${useImagePreloading.name}`, () => {
From 672414c614134ccc44e4936aeef36a4dd8add635 Mon Sep 17 00:00:00 2001
From: Parkreiner <michaelsmith@coder.com>
Date: Wed, 4 Oct 2023 17:08:56 +0000
Subject: [PATCH 5/5] fix: clean up logic with images
---
site/src/hooks/images.ts | 34 ++++++++++++++++++++++++----------
1 file changed, 24 insertions(+), 10 deletions(-)
diff --git a/site/src/hooks/images.ts b/site/src/hooks/images.ts
index 0d329eb125305..380154a82fb4f 100644
--- a/site/src/hooks/images.ts
+++ b/site/src/hooks/images.ts
@@ -13,7 +13,7 @@ const MAX_RETRIES = 3;
*/
type ImageTrackerEntry = {
image: HTMLImageElement;
- status: "loading" | "error" | "success";
+ status: "loading" | "aborted" | "error" | "success";
retries: number;
};
@@ -46,8 +46,10 @@ function preloadImages(imageUrls?: readonly string[]): () => void {
dummyImage.src = imgUrl;
const entry: ImageTrackerEntry = {
+ // Actual HTMLImageElement treats empty strings as an error, but that's
+ // not a concern for the tracker
+ status: imgUrl === "" ? "aborted" : "loading",
image: dummyImage,
- status: "loading",
retries: 0,
};
@@ -56,7 +58,7 @@ function preloadImages(imageUrls?: readonly string[]): () => void {
};
dummyImage.onerror = () => {
- if (imgUrl !== "") {
+ if (imgUrl !== "aborted") {
entry.status = "error";
}
};
@@ -66,19 +68,31 @@ function preloadImages(imageUrls?: readonly string[]): () => void {
}
const skipRetry =
- prevEntry.status === "loading" ||
- prevEntry.status === "success" ||
- prevEntry.retries === MAX_RETRIES;
+ prevEntry.status !== "error" || prevEntry.retries === MAX_RETRIES;
if (skipRetry) {
continue;
}
+ // Have to disconnect error handler while resetting the src, or else an
+ // error will be processed
+ const prevErrorHandler = prevEntry.image.onerror;
+ prevEntry.image.onerror = null;
prevEntry.image.src = "";
- const retryId = window.setTimeout(() => {
- prevEntry.image.src = imgUrl;
- prevEntry.retries++;
- }, 0);
+
+ const retryId = window.setTimeout(
+ () => {
+ prevEntry.image.src = imgUrl;
+ prevEntry.image.onerror = prevErrorHandler;
+
+ prevEntry.status = "loading";
+ prevEntry.retries++;
+ },
+ // Zero is intentional; only purpose of the timeout call is to make the
+ // update logic take a brief trip through the task queue while making it
+ // easy to cancel the operation
+ 0,
+ );
retryTimeoutIds.push(retryId);
}