From 9462211e29f5476dbd373871604424f4547c4f25 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/session.js | 24 +++++- src/tests/fixtures/frame_preloading.html | 6 ++ src/tests/fixtures/frames/preloading.html | 3 +- src/tests/fixtures/preloading.html | 12 ++- src/tests/functional/preloader_tests.js | 99 +++++++++++++++-------- src/tests/server.mjs | 6 +- 7 files changed, 123 insertions(+), 50 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/session.js b/src/core/session.js index 44edc7856..84ce43a9d 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,24 @@ export class Session { return this.history.restorationIdentifier } + // Preloader delegate + + shouldPreloadLink(element) { + const location = new URL(element.href) + const isUnsafe = element.hasAttribute("data-turbo-method") + const isStream = element.hasAttribute("data-turbo-stream") + const frameTarget = element.getAttribute("data-turbo-frame") + const frame = frameTarget == "_top" ? + null : + document.getElementById(frameTarget) || findClosestRecursively(element, "turbo-frame:not([disabled])") + + if (isUnsafe || isStream || frame instanceof FrameElement) { + return false + } else { + return this.elementIsNavigatable(element) && locationIsVisitable(location, this.snapshot.rootLocation) + } + } + // History delegate historyPoppedToLocationWithRestorationIdentifier(location, restorationIdentifier) { diff --git a/src/tests/fixtures/frame_preloading.html b/src/tests/fixtures/frame_preloading.html index 9fb02266f..06aa6210f 100644 --- a/src/tests/fixtures/frame_preloading.html +++ b/src/tests/fixtures/frame_preloading.html @@ -8,5 +8,11 @@ + + + Navigate #hello + + + Navigate #hello from the outside diff --git a/src/tests/fixtures/frames/preloading.html b/src/tests/fixtures/frames/preloading.html index b4f56836a..a642b5ea7 100644 --- a/src/tests/fixtures/frames/preloading.html +++ b/src/tests/fixtures/frames/preloading.html @@ -8,8 +8,7 @@ - Visit preloaded - page + Visit preloaded page diff --git a/src/tests/fixtures/preloading.html b/src/tests/fixtures/preloading.html index ad3a6d0b2..9bdacc1aa 100644 --- a/src/tests/fixtures/preloading.html +++ b/src/tests/fixtures/preloading.html @@ -8,9 +8,19 @@ - + Visit preloaded page + + POST + + [data-turbo-stream] + +
+ Visit page +
+ + Navigate off-site diff --git a/src/tests/functional/preloader_tests.js b/src/tests/functional/preloader_tests.js index ecd7ca619..c303ddba4 100644 --- a/src/tests/functional/preloader_tests.js +++ b/src/tests/functional/preloader_tests.js @@ -1,53 +1,86 @@ 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) + + assert.ok(await urlInSnapshotCache(page, href)) +}) + +test("test preloads anchor from frame that will drive the page", async ({ page }) => { + await page.goto("/src/tests/fixtures/frame_preloading.html") + await nextEventOnTarget(page, "menu", "turbo:frame-load") + + const preloadLink = await page.locator("#menu a[data-turbo-frame=_top]") + const href = await preloadLink.evaluate((link) => link.href) + + assert.ok(await urlInSnapshotCache(page, href)) +}) + +test("test does not preload anchor off-site", async ({ page }) => { + await page.goto("/src/tests/fixtures/preloading.html") + + const link = await page.locator("a[href*=https]") + const href = await link.evaluate((link) => link.href) + + assert.notOk(await urlInSnapshotCache(page, href)) +}) + +test("test does not preload anchor that will drive an ancestor frame", async ({ page }) => { + await page.goto("/src/tests/fixtures/frame_preloading.html") + + const preloadLink = await page.locator("#hello a[data-turbo-preload]") + const href = await preloadLink.evaluate((link) => link.href) - return await cache.has(preloadedUrl) - }) - ) + assert.notOk(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"]` +test("test does not preload anchor that will drive a target frame", async ({ page }) => { await page.goto("/src/tests/fixtures/frame_preloading.html") - await page.waitForSelector("#frame_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("a[data-turbo-frame]") + const href = await preloadLink.evaluate((link) => link.href) + + assert.notOk(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) - return await cache.has(preloadedUrl) - }) - ) + 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)