From 06a6bfdc13a05881fe7bd41a5272b2f9ae48220e Mon Sep 17 00:00:00 2001 From: Sean Doyle Date: Tue, 30 Jan 2024 22:31:51 -0500 Subject: [PATCH] `LinkPrefetchObserver`: listen for complementary events Prior to this commit, the `LinkPrefetchObserver` only listened for `mouseleave` events to clear the `PrefetchCache` instance. Not only were `mouseenter` events excluded, but the `mouseleave` event listeners were attached directly to the `` element with a `{ once: true }` option. While unlikely, its was for those event listeners to never be removed if a `mouseleave` were to not fire. Similarly, during `touchstart` events the event listener were added, but never removed since there wasn't a complementary `touchend` or `touchcancel` event firing to remove it. This commit makes two changes to the event listeners: 1. extract the `addEventListener` calls to a loop, looping over `mouseenter` and `touchstart` event names 2. define complementary events for both `mouseenter` (`mouseleave`) and `touchstart` (`touchend` and `touchcancel`) By moving the cancellation logic out of individual event listeners and into an `this.eventTarget`-wide scope, we limit the risk of leaking listeners. Similarly, we only ever instantiate one per event-pairing. To track the `` element reference, define both a `this.#tryToCancelPrefetchRequest` method and a `this.#linkToPrefetch` property to hold the reference to the `` element in question. --- src/observers/link_prefetch_observer.js | 60 +++++++++++++------ .../link_prefetch_observer_tests.js | 30 +++------- 2 files changed, 51 insertions(+), 39 deletions(-) diff --git a/src/observers/link_prefetch_observer.js b/src/observers/link_prefetch_observer.js index 165569dbd..4488ffd55 100644 --- a/src/observers/link_prefetch_observer.js +++ b/src/observers/link_prefetch_observer.js @@ -9,10 +9,14 @@ import { StreamMessage } from "../core/streams/stream_message" import { FetchMethod, FetchRequest } from "../http/fetch_request" import { prefetchCache, cacheTtl } from "../core/drive/prefetch_cache" +const observedEvents = { + "mouseenter": ["mouseleave"], + "touchstart": ["touchend", "touchcancel"] +} + export class LinkPrefetchObserver { started = false - hoverTriggerEvent = "mouseenter" - touchTriggerEvent = "touchstart" + #prefetchedLink = null constructor(delegate, eventTarget) { this.delegate = delegate @@ -32,26 +36,34 @@ export class LinkPrefetchObserver { stop() { if (!this.started) return - this.eventTarget.removeEventListener(this.hoverTriggerEvent, this.#tryToPrefetchRequest, { - capture: true, - passive: true - }) - this.eventTarget.removeEventListener(this.touchTriggerEvent, this.#tryToPrefetchRequest, { - capture: true, - passive: true + Object.entries(observedEvents).forEach(([startEventName, stopEventNames]) => { + this.eventTarget.removeEventListener(startEventName, this.#tryToPrefetchRequest, { + capture: true, + passive: true + }) + stopEventNames.forEach((stopEventName) => { + this.eventTarget.removeEventListener(stopEventName, this.#tryToCancelPrefetchRequest, { + capture: true, + passive: true + }) + }) }) this.eventTarget.removeEventListener("turbo:before-fetch-request", this.#tryToUsePrefetchedRequest, true) this.started = false } #enable = () => { - this.eventTarget.addEventListener(this.hoverTriggerEvent, this.#tryToPrefetchRequest, { - capture: true, - passive: true - }) - this.eventTarget.addEventListener(this.touchTriggerEvent, this.#tryToPrefetchRequest, { - capture: true, - passive: true + Object.entries(observedEvents).forEach(([startEventName, stopEventNames]) => { + this.eventTarget.addEventListener(startEventName, this.#tryToPrefetchRequest, { + capture: true, + passive: true + }) + stopEventNames.forEach((stopEventName) => { + this.eventTarget.addEventListener(stopEventName, this.#tryToCancelPrefetchRequest, { + capture: true, + passive: true + }) + }) }) this.eventTarget.addEventListener("turbo:before-fetch-request", this.#tryToUsePrefetchedRequest, true) this.started = true @@ -68,6 +80,7 @@ export class LinkPrefetchObserver { const location = getLocationForLink(link) if (this.delegate.canPrefetchRequestToLocation(link, location)) { + this.#prefetchedLink = link const fetchRequest = new FetchRequest( this, FetchMethod.get, @@ -77,12 +90,23 @@ export class LinkPrefetchObserver { ) prefetchCache.setLater(location.toString(), fetchRequest, this.#cacheTtl) - - link.addEventListener("mouseleave", () => prefetchCache.clear(), { once: true }) } } } + #tryToCancelPrefetchRequest = (event) => { + const targets = "targetTouches" in event ? + [event.target, ...event.targetTouches] : + [event.target] + const leavingPrefetchedLink = targets.some((target) => target === this.#prefetchedLink) + + if (leavingPrefetchedLink) { + prefetchCache.clear() + + this.#prefetchedLink = null + } + } + #tryToUsePrefetchedRequest = (event) => { if (event.target.tagName !== "FORM" && event.detail.fetchOptions.method === "get") { const cached = prefetchCache.get(event.detail.url.toString()) diff --git a/src/tests/functional/link_prefetch_observer_tests.js b/src/tests/functional/link_prefetch_observer_tests.js index 584d673ef..281f51083 100644 --- a/src/tests/functional/link_prefetch_observer_tests.js +++ b/src/tests/functional/link_prefetch_observer_tests.js @@ -226,7 +226,15 @@ test("it resets the cache when a link is hovered", async ({ page }) => { test("it prefetches page on touchstart", async ({ page }) => { await goTo({ page, path: "/hover_to_prefetch.html" }) - await assertPrefetchedOnTouchstart({ page, selector: "#anchor_for_prefetch" }) + + let requestMade = false + const link = page.locator("#anchor_for_prefetch") + page.on("request", (request) => (requestMade = true)) + + await link.tap() + await sleep(100) + + assertRequestMade(requestMade) }) test("it does not make a network request when clicking on a link that has been prefetched", async ({ page }) => { @@ -248,26 +256,6 @@ test("it follows the link using the cached response when clicking on a link that assert.equal(await page.title(), "Prefetched Page") }) -const assertPrefetchedOnTouchstart = async ({ page, selector, callback }) => { - let requestMade = false - - page.on("request", (request) => { - callback && callback(request) - requestMade = true - }) - - const selectorXY = await page.$eval(selector, (el) => { - const { x, y } = el.getBoundingClientRect() - return { x, y } - }) - - await page.touchscreen.tap(selectorXY.x, selectorXY.y) - - await sleep(100) - - assertRequestMade(requestMade) -} - const assertPrefetchedOnHover = async ({ page, selector, callback }) => { let requestMade = false