Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LinkPrefetchObserver: listen for complementary events #1147

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 47 additions & 18 deletions src/observers/link_prefetch_observer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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", "touchmove", "touchcancel"]
}

export class LinkPrefetchObserver {
started = false
hoverTriggerEvent = "mouseenter"
touchTriggerEvent = "touchstart"
#prefetchedLink = null

constructor(delegate, eventTarget) {
this.delegate = delegate
Expand All @@ -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
Expand All @@ -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,
Expand All @@ -77,12 +90,28 @@ export class LinkPrefetchObserver {
)

prefetchCache.setLater(location.toString(), fetchRequest, this.#cacheTtl)

link.addEventListener("mouseleave", () => prefetchCache.clear(), { once: true })
Copy link
Contributor Author

@seanpdoyle seanpdoyle Feb 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@afcapel this line is the crux of this PR. Adding listeners directly to the <a> without removing them poses a risk of leaking memory from the instances. Additionally, there isn't a parallel pattern for touchstart (and touchend) events, so I'm not sure that cleanup is currently occurring on touch devices.

The rest of the changes in this diff support that setup-and-teardown.

Once this is merged, adding support for focusin and focusout events would be fairly trivial.

Copy link
Contributor Author

@seanpdoyle seanpdoyle Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@afcapel if this is going to be enabled by default, I think this change is important to consider for touch devices.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding listeners directly to the < a > without removing them poses a risk of leaking memory from the instances.

When once is true the listener would be automatically removed when invoked. So there wouldn't be a risk of memory leaks. Or am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh just read the description, got it. Sorry, was linked directly to this comment from #1167 😃

}
}
}

#tryToCancelPrefetchRequest = (event) => {
let hasLeftLink = false

if (event.type === "touchend") {
hasLeftLink = Array.from(event.changedTouches).some((target) => target === this.#prefetchedLink)
} else if (event.type === "touchmove") {
hasLeftLink = !Array.from(event.targetTouches).some((target) => target === this.#prefetchedLink)
} else {
hasLeftLink = event.target !== this.#prefetchedLink
}

if (hasLeftLink) {
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())
Expand Down
30 changes: 9 additions & 21 deletions src/tests/functional/link_prefetch_observer_tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 }) => {
Expand All @@ -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

Expand Down
Loading