Skip to content

Commit

Permalink
Only cache the link you're currently hovering
Browse files Browse the repository at this point in the history 
Instead of maintaining a cache of all the links that have been hovered
in the last 10 seconds.

This solves issues where the user hovers a link, then performs a non-safe
action and then later clicks the link. In this case, we would be showing
stale content from before the action was performed.
  • Loading branch information
@afcapel
afcapel committed Jan 18, 2024
1 parent e87bf75 commit fb58b6b
Show file tree
Hide file tree
Showing 8 changed files with 64 additions and 239 deletions.
1 change: 0 additions & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@
"body-parser": "^1.20.1",
"chai": "~4.3.4",
"eslint": "^8.13.0",
"eta": "^3.2.0",
"express": "^4.18.2",
"idiomorph": "^0.3.0",
"multer": "^1.4.2",
Expand Down
34 changes: 33 additions & 1 deletion src/core/drive/prefetch_cache.js
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,35 @@
export const prefetchCache = new Map()

const PREFETCH_DELAY = 100

class PrefetchCache {
#prefetchTimeout = null
#prefetched = null

get(url) {
if (this.#prefetched && this.#prefetched.url === url && this.#prefetched.expire > Date.now()) {
return this.#prefetched.request
}
}

setLater(url, request, ttl) {
this.clear()

this.#prefetchTimeout = setTimeout(() => {
request.perform()
this.set(url, request, ttl)
this.#prefetchTimeout = null
}, PREFETCH_DELAY)
}

set(url, request, ttl) {
this.#prefetched = { url, request, expire: new Date(new Date().getTime() + ttl) }
}

clear() {
if (this.#prefetchTimeout) clearTimeout(this.#prefetchTimeout)
this.#prefetched = null
}
}

export const cacheTtl = 10 * 1000
export const prefetchCache = new PrefetchCache()
28 changes: 2 additions & 26 deletions src/core/session.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ import { PageView } from "./drive/page_view"
import { FrameElement } from "../elements/frame_element"
import { Preloader } from "./drive/preloader"
import { Cache } from "./cache"
import { prefetchCache } from "./drive/prefetch_cache"
import { FetchMethod, FetchRequest } from "../http/fetch_request"

export class Session {
navigator = new Navigator(this)
Expand Down Expand Up @@ -207,33 +205,11 @@ export class Session {

// Link hover observer delegate

canPrefetchAndCacheRequestToLocation(link, location, event) {
const absoluteUrl = location.toString()
const cached = prefetchCache.get(absoluteUrl)

canPrefetchRequestToLocation(link, location) {
return (
this.elementIsNavigatable(link) &&
locationIsVisitable(location, this.snapshot.rootLocation) &&
(!cached || cached.ttl < new Date())
)
}

prefetchAndCacheRequestToLocation(link, location, cacheTtl) {
const absoluteUrl = location.toString()
const fetchRequest = new FetchRequest(
this.linkPrefetchObserver,
FetchMethod.get,
location,
new URLSearchParams(),
link
locationIsVisitable(location, this.snapshot.rootLocation)
)

fetchRequest.perform()

prefetchCache.set(absoluteUrl, {
fetchRequest,
ttl: new Date(new Date().getTime() + cacheTtl)
})
}

// Link click observer delegate
Expand Down
2 changes: 1 addition & 1 deletion src/observers/form_link_click_observer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export class FormLinkClickObserver {

// Link hover observer delegate

canPrefetchAndCacheRequestToLocation(link, location, event) {
canPrefetchRequestToLocation(link, location) {
return false
}

Expand Down
46 changes: 20 additions & 26 deletions src/observers/link_prefetch_observer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import {
getMetaContent,
findClosestRecursively
} from "../util"

import { FetchMethod, FetchRequest } from "../http/fetch_request"
import { prefetchCache, cacheTtl } from "../core/drive/prefetch_cache"

export class LinkPrefetchObserver {
Expand Down Expand Up @@ -63,40 +65,32 @@ export class LinkPrefetchObserver {

if (isLink && this.#isPrefetchable(target)) {
const link = target

this.prefetchTimeout = setTimeout(() => {
this.#startPrefetch(event, link)
} , this.delayBeforePrefetching)

link.addEventListener("mouseleave", this.#cancelPrefetchTimeoutIfAny, {
capture: true,
passive: true
})
}
}

#startPrefetch = (event, link) => {
const location = getLocationForLink(link)
if (this.delegate.canPrefetchAndCacheRequestToLocation(link, location, event)) {
this.delegate.prefetchAndCacheRequestToLocation(link, location, this.#cacheTtl)
}
}

#cancelPrefetchTimeoutIfAny = () => {
if (this.prefetchTimeout) {
clearTimeout(this.prefetchTimeout)
const location = getLocationForLink(link)

if (this.delegate.canPrefetchRequestToLocation(link, location)) {
const fetchRequest = new FetchRequest(
this,
FetchMethod.get,
location,
new URLSearchParams(),
target
)

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

#tryToUsePrefetchedRequest = (event) => {
if (event.target.tagName !== "FORM" && event.detail.fetchOptions.method === "get") {
const cached = prefetchCache.get(event.detail.url.toString())

if (cached && cached.ttl > new Date()) {
// User clicked link, use cache response and clear cache.
event.detail.fetchRequest = cached.fetchRequest
prefetchCache.clear()
if (cached) {
// User clicked link, use cache response
event.detail.fetchRequest = cached
}

prefetchCache.clear()
}
}

Expand Down
64 changes: 8 additions & 56 deletions src/tests/functional/link_prefetch_observer_tests.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,53 +169,22 @@ test("it prefetches links with a delay", async ({ page }) => {
assertRequestMade(requestMade)
})

test("it cancels the prefetch request if the link is no longer hovered", async ({ page }) => {
test("it resets the cache when a link is hovered", async ({ page }) => {
await goTo({ page, path: "/hover_to_prefetch.html" })

let requestMade = false
page.on("request", async (request) => (requestMade = true))
let requestCount = 0
page.on("request", async () => (requestCount++))

await page.hover("#anchor_for_prefetch")
await sleep(75)

assertRequestNotMade(requestMade)
await sleep(200)

assert.equal(requestCount, 1)
await page.mouse.move(0, 0)

await sleep(100)

assertRequestNotMade(requestMade)
})

test("it clears cache on form submission", async ({ page }) => {
// eslint-disable-next-line no-undef
await page.goto(`/__turbo/posts?worker_id=${process.env.TEST_PARALLEL_INDEX}`)

// Create a post
await page.click("#submit")
const post = await page.getByTestId("post_1")
const postText = await post.innerText()
assert.equal(postText, "A new post title")

// Visit the post
await post.click()

// Hover over the link back to all posts, prefetching page showing 0 comments
await page.hover("#anchor_back_to_all_posts")

// Create a comment, prefetch cache should be discarded
await page.click("#comment_submit")
const comment = await page.getByTestId("comment_1")
const commentText = await comment.innerText()
assert.equal(commentText, "A new comment")

// Go back to all posts
await page.click("#anchor_back_to_all_posts")
await page.hover("#anchor_for_prefetch")
await sleep(200)

// Assert that the comments count is 1, not 0
const postComments = await page.getByTestId("post_1_comments")
const postCommentsText = await postComments.innerText()
assert.equal(postCommentsText, "(1 comments)")
assert.equal(requestCount, 2)
})

test("it prefetches page on touchstart", async ({ page }) => {
Expand All @@ -232,23 +201,6 @@ test("it does not make a network request when clicking on a link that has been p
await assertNotPrefetchedOnHover({ page, selector: "#anchor_for_prefetch" })
})

test("it does not more than 2 network requests when hovering between 2 links", async ({ page }) => {
await goTo({ page, path: "/hover_to_prefetch.html" })

let requestCount = 0

page.on("request", async (request) => {
requestCount++
})

await hoverSelector({ page, selector: "#anchor_for_prefetch" })
await hoverSelector({ page, selector: "#anchor_for_prefetch_other_href" })
await hoverSelector({ page, selector: "#anchor_for_prefetch" })
await hoverSelector({ page, selector: "#anchor_for_prefetch_other_href" })

assert.equal(requestCount, 2)
})

test("it follows the link using the cached response when clicking on a link that has been prefetched", async ({
page
}) => {
Expand Down
123 changes: 0 additions & 123 deletions src/tests/server.mjs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import multer from "multer"
import path from "path"
import url, { fileURLToPath } from "url"
import fs from "fs"
import { Eta } from "eta"

const __filename = fileURLToPath(import.meta.url)
const __dirname = path.dirname(__filename)
Expand All @@ -13,7 +12,6 @@ const { json, urlencoded } = bodyParser

const router = Router()
const streamResponses = new Set()
const templateRenderer = new Eta({ views: path.join(__dirname, "templates") })

router.use(multer().none())

Expand Down Expand Up @@ -153,127 +151,6 @@ router.put("/messages/:id", (request, response) => {
}
})

router.get("/posts", async (request, response) => {
const { worker_id } = request.query

const posts = await getPosts(worker_id)

const res = templateRenderer.render("./posts", { posts, worker_id })

response.type("html").status(200).send(res)
})

router.post("/posts", async (request, response) => {
const { title, body, worker_id } = request.body

await addPost(title, body, worker_id)

response.redirect(303, `/__turbo/posts?worker_id=${worker_id}`)
})

router.get("/posts/:id/", async (request, response) => {
const { worker_id } = request.query
const { id } = request.params

const posts = await getPosts(worker_id)
const post = posts.find((post) => post.id == id)

if (post) {
const res = templateRenderer.render("./post", { post, worker_id })

response.type("html").status(200).send(res)
} else {
response.sendStatus(404)
}
})

router.post("/posts/:id", async (request, response) => {
if (request.body._method == "delete") {
const { worker_id } = request.body
const { id } = request.params

await deletePost(id, worker_id)
response.redirect(303, `/__turbo/posts?worker_id=${worker_id}`)
} else {
response.sendStatus(422)
}
})

router.post("/posts/:post_id/comments", async (request, response) => {
const { post_id } = request.params
const { worker_id } = request.body
const { body } = request.body.comment

const post = await getPost(post_id, worker_id)

if (post) {
await addComment(post_id, body, worker_id)

response.redirect(303, `/__turbo/posts/${post_id}?worker_id=${worker_id}`)
} else {
response.sendStatus(404)
}
})

const postsDatabaseName = (worker_id) => {
return `src/tests/fixtures/volatile_posts_database_${worker_id}.json`
}

const ensureDatabase = async (worker_id) => {
if (worker_id == null || worker_id == undefined) {
throw new Error("worker_id is required")
}

if (!fs.existsSync(postsDatabaseName(worker_id))) {
fs.writeFileSync(postsDatabaseName(worker_id), "[]")
}
}

const getPosts = async (worker_id) => {
await ensureDatabase(worker_id)

return JSON.parse(fs.readFileSync(postsDatabaseName(worker_id)).toString())
}

const addPost = async (title, body, worker_id) => {
await ensureDatabase(worker_id)
const posts = await getPosts(worker_id)

const id = posts.length + 1

posts.push({ id, title, body })

return fs.writeFileSync(postsDatabaseName(worker_id), JSON.stringify(posts))
}

const getPost = async (id, worker_id) => {
await ensureDatabase(worker_id)
const posts = await getPosts(worker_id)

return posts.find((post) => post.id == id)
}

const deletePost = async (id, worker_id) => {
await ensureDatabase(worker_id)
const posts = await getPosts(worker_id)
const newPosts = posts.filter((post) => post.id != id)

return fs.writeFileSync(postsDatabaseName(worker_id), JSON.stringify(newPosts))
}

const addComment = async (postId, body, worker_id) => {
await ensureDatabase(worker_id)
const posts = await getPosts(worker_id)
const post = posts.find((post) => post.id == postId)
const comments = post.comments || []
const id = comments.length + 1

post.comments = comments
post.comments.push({ id, body })

return fs.writeFileSync(postsDatabaseName(worker_id), JSON.stringify(posts))
}

router.get("/messages", (request, response) => {
response.set({
"Cache-Control": "no-cache",
Expand Down
Loading

0 comments on commit fb58b6b

Please sign in to comment.