From da137e87d6f42675641eb6a0187c79160160c680 Mon Sep 17 00:00:00 2001 From: Sean Doyle Date: Thu, 12 Oct 2023 11:03:03 -0400 Subject: [PATCH] Guard `[data-turbo-preload]` with conditionals Prior to this change, _any_ `` element with `[data-turbo-preload]` would be preloaded. With that behavior comes some risk: * if an element is nested within a `[data-turbo="false"]` element, the preloading would occur unnecessarily * if an element has `[data-turbo-stream]`, the preloaded request won't be the same as the ensuing request due to differences in the `Accept:` header * if an element has `[data-turbo-method]`, the preloaded request won't be the same as the ensuing request due to differences in the method * if an element is within a `` or driving a frame via `[data-turbo-frame]`, the preloaded request won't be the same as the ensuing request due to differences in the `Turbo-Frame:` header This commit extends the `Preloader` delegate interface to include a `shouldPreloadLink(link: HTMLAnchorElement)` predicate method to give delegates an opportunity to opt-out of preloading. The `Session` implementation of the delegate class adds rejects `` elements that match any of those cases. --- src/core/drive/preloader.js | 23 ++++---- src/core/frames/frame_controller.js | 6 +++ src/core/session.js | 19 ++++++- src/tests/fixtures/preloading.html | 8 +++ src/tests/functional/preloader_tests.js | 70 ++++++++++++++----------- src/tests/server.mjs | 6 +-- 6 files changed, 86 insertions(+), 46 deletions(-) diff --git a/src/core/drive/preloader.js b/src/core/drive/preloader.js index b971cea19..ab7bc2efa 100644 --- a/src/core/drive/preloader.js +++ b/src/core/drive/preloader.js @@ -3,27 +3,28 @@ import { PageSnapshot } from "./page_snapshot" export class Preloader { selector = "a[data-turbo-preload]" - constructor(delegate) { + constructor(delegate, snapshotCache) { this.delegate = delegate - } - - get snapshotCache() { - return this.delegate.navigator.view.snapshotCache + this.snapshotCache = snapshotCache } start() { if (document.readyState === "loading") { - return document.addEventListener("DOMContentLoaded", () => { - this.preloadOnLoadLinksForView(document.body) - }) + document.addEventListener("DOMContentLoaded", this.#preloadAll) } else { this.preloadOnLoadLinksForView(document.body) } } + stop() { + document.removeEventListener("DOMContentLoaded", this.#preloadAll) + } + preloadOnLoadLinksForView(element) { for (const link of element.querySelectorAll(this.selector)) { - this.preloadURL(link) + if (this.delegate.shouldPreloadLink(link)) { + this.preloadURL(link) + } } } @@ -42,4 +43,8 @@ export class Preloader { // If we cannot preload that is ok! } } + + #preloadAll = () => { + this.preloadOnLoadLinksForView(document.body) + } } diff --git a/src/core/frames/frame_controller.js b/src/core/frames/frame_controller.js index 1cab2902e..6c8e78a85 100644 --- a/src/core/frames/frame_controller.js +++ b/src/core/frames/frame_controller.js @@ -284,6 +284,12 @@ export class FrameController { viewInvalidated() {} + // Preloader delegate + + shouldPreloadLink(_element) { + return false + } + // Frame renderer delegate willRenderFrame(currentElement, _newElement) { diff --git a/src/core/session.js b/src/core/session.js index 44edc7856..30bd8ea02 100644 --- a/src/core/session.js +++ b/src/core/session.js @@ -20,7 +20,6 @@ import { Preloader } from "./drive/preloader" export class Session { navigator = new Navigator(this) history = new History(this) - preloader = new Preloader(this) view = new PageView(this, document.documentElement) adapter = new BrowserAdapter(this) @@ -40,6 +39,10 @@ export class Session { started = false formMode = "on" + constructor() { + this.preloader = new Preloader(this, this.view.snapshotCache) + } + start() { if (!this.started) { this.pageObserver.start() @@ -72,6 +75,7 @@ export class Session { this.streamObserver.stop() this.frameRedirector.stop() this.history.stop() + this.preloader.stop() this.started = false } } @@ -123,6 +127,19 @@ export class Session { return this.history.restorationIdentifier } + // Preloader delegate + + shouldPreloadLink(element) { + const isUnsafe = element.hasAttribute("data-turbo-method") + const isStream = element.hasAttribute("data-turbo-stream") + + if (isUnsafe || isStream) { + return false + } else { + return this.elementIsNavigatable(element) + } + } + // History delegate historyPoppedToLocationWithRestorationIdentifier(location, restorationIdentifier) { diff --git a/src/tests/fixtures/preloading.html b/src/tests/fixtures/preloading.html index ad3a6d0b2..dbc39c782 100644 --- a/src/tests/fixtures/preloading.html +++ b/src/tests/fixtures/preloading.html @@ -11,6 +11,14 @@ Visit preloaded page + + POST + + [data-turbo-stream] + +
+ Visit page +
diff --git a/src/tests/functional/preloader_tests.js b/src/tests/functional/preloader_tests.js index ecd7ca619..acb8d5b25 100644 --- a/src/tests/functional/preloader_tests.js +++ b/src/tests/functional/preloader_tests.js @@ -1,53 +1,59 @@ import { test } from "@playwright/test" import { assert } from "chai" -import { nextBeat } from "../helpers/page" +import { nextEventOnTarget } from "../helpers/page" test("test preloads snapshot on initial load", async ({ page }) => { - // contains `a[rel="preload"][href="http://localhost:9000/src/tests/fixtures/preloaded.html"]` await page.goto("/src/tests/fixtures/preloading.html") - await nextBeat() - assert.ok( - await page.evaluate(async () => { - const preloadedUrl = new URL("http://localhost:9000/src/tests/fixtures/preloaded.html") - const cache = window.Turbo.session.preloader.snapshotCache + const preloadLink = await page.locator("#preload_anchor") + const href = await preloadLink.evaluate((link) => link.href) - return await cache.has(preloadedUrl) - }) - ) + assert.ok(await urlInSnapshotCache(page, href)) }) test("test preloads snapshot on page visit", async ({ page }) => { - // contains `a[rel="preload"][href="http://localhost:9000/src/tests/fixtures/preloading.html"]` await page.goto("/src/tests/fixtures/hot_preloading.html") - - // contains `a[rel="preload"][href="http://localhost:9000/src/tests/fixtures/preloaded.html"]` await page.click("#hot_preload_anchor") - await page.waitForSelector("#preload_anchor") - await nextBeat() - assert.ok( - await page.evaluate(async () => { - const preloadedUrl = new URL("http://localhost:9000/src/tests/fixtures/preloaded.html") - const cache = window.Turbo.session.preloader.snapshotCache + const preloadLink = await page.locator("#preload_anchor") + const href = await preloadLink.evaluate((link) => link.href) - return await cache.has(preloadedUrl) - }) - ) + assert.ok(await urlInSnapshotCache(page, href)) }) test("test navigates to preloaded snapshot from frame", async ({ page }) => { - // contains `a[rel="preload"][href="http://localhost:9000/src/tests/fixtures/preloaded.html"]` await page.goto("/src/tests/fixtures/frame_preloading.html") - await page.waitForSelector("#frame_preload_anchor") - await nextBeat() + await nextEventOnTarget(page, "menu", "turbo:frame-load") - assert.ok( - await page.evaluate(async () => { - const preloadedUrl = new URL("http://localhost:9000/src/tests/fixtures/preloaded.html") - const cache = window.Turbo.session.preloader.snapshotCache + const preloadLink = await page.locator("#frame_preload_anchor") + const href = await preloadLink.evaluate((link) => link.href) - return await cache.has(preloadedUrl) - }) - ) + assert.ok(await urlInSnapshotCache(page, href)) }) + +test("test does not preload a link with [data-turbo=false]", async ({ page }) => { + await page.goto("/src/tests/fixtures/preloading.html") + + const link = await page.locator("[data-turbo=false] a") + const href = await link.evaluate((link) => link.href) + + assert.notOk(await urlInSnapshotCache(page, href)) +}) + +test("test does not preload a link with [data-turbo-method]", async ({ page }) => { + await page.goto("/src/tests/fixtures/preloading.html") + + const preloadLink = await page.locator("a[data-turbo-method]") + const href = await preloadLink.evaluate((link) => link.href) + + assert.notOk(await urlInSnapshotCache(page, href)) +}) + +function urlInSnapshotCache(page, href) { + return page.evaluate((href) => { + const preloadedUrl = new URL(href) + const cache = window.Turbo.session.preloader.snapshotCache + + return cache.has(preloadedUrl) + }, href) +} diff --git a/src/tests/server.mjs b/src/tests/server.mjs index 0628f2e30..fc4b7b369 100644 --- a/src/tests/server.mjs +++ b/src/tests/server.mjs @@ -1,10 +1,8 @@ -import { Router } from "express" -import express from "express" +import express, { Router } from "express" import bodyParser from "body-parser" import multer from "multer" import path from "path" -import url from "url" -import { fileURLToPath } from 'url' +import url, { fileURLToPath } from "url" import fs from "fs" const __filename = fileURLToPath(import.meta.url)