Guard [data-turbo-preload] with conditionals

Prior to this change, _any_ `<a>` 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 `<turbo-frame>` 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 `<a>` elements that match any of those cases.
seanpdoyle · Oct 12, 2023 · 279bbfc · 279bbfc
1 parent c207f5b
commit 279bbfc
Show file tree

Hide file tree

Showing 7 changed files with 112 additions and 50 deletions.
diff --git 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
@@ -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,21 @@ 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")
+    const closestFrame = findClosestRecursively(element, "turbo-frame:not([disabled])")
+    const targetFrame = document.getElementById(element.getAttribute("data-turbo-frame"))
+
+    if (isUnsafe || isStream || closestFrame instanceof FrameElement || targetFrame 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
@@ -8,5 +8,9 @@
   </head>
   <body>
     <turbo-frame id="menu" src="/src/tests/fixtures/frames/preloading.html"></turbo-frame>
+
+    <turbo-frame id="hello">
+      <a href="/src/tests/fixtures/frames/hello.html" data-turbo-preload>Navigate #hello</a>
+    </turbo-frame>
   </body>
 </html>
diff --git a/src/tests/fixtures/frames/preloading.html b/src/tests/fixtures/frames/preloading.html
@@ -8,8 +8,9 @@
   </head>
   <body>
     <turbo-frame id="menu">
-      <a href="/src/tests/fixtures/preloaded.html" id="frame_preload_anchor" data-turbo-preload="true">Visit preloaded
-        page</a>
+      <a href="/src/tests/fixtures/preloaded.html" id="frame_preload_anchor" data-turbo-preload data-turbo-frame="_top">Visit preloaded page</a>
+
+      <a href="/src/tests/fixtures/frames.html" id="frame_preload_anchor" data-turbo-preload data-turbo-frame="_top">Visit preloaded page</a>
     </turbo-frame>
   </body>
 </html>
diff --git a/src/tests/fixtures/preloading.html b/src/tests/fixtures/preloading.html
@@ -8,9 +8,19 @@
 </head>
 
 <body>
-  <a href="/src/tests/fixtures/preloaded.html" id="preload_anchor" data-turbo-preload="true">
+  <a href="/src/tests/fixtures/preloaded.html" id="preload_anchor" data-turbo-preload>
     Visit preloaded page
   </a>
+
+  <a href="/__turbo/redirect?path=/src/tests/fixtures/one.html" data-turbo-method="post" data-turbo-preload>POST</a>
+
+  <a href="/src/tests/fixtures/one.html" data-turbo-stream data-turbo-preload>[data-turbo-stream]</a>
+
+  <div data-turbo="false">
+    <a href="/src/tests/fixtures/one.html" data-turbo-preload>Visit page</a>
+  </div>
+
+  <a href="https://example.com" data-turbo-preload>Navigate off-site</a>
 </body>
 
 </html>
diff --git a/src/tests/functional/preloader_tests.js b/src/tests/functional/preloader_tests.js
@@ -1,53 +1,78 @@
 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)
 
-      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"]`
+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 frame", async ({ page }) => {
   await page.goto("/src/tests/fixtures/frame_preloading.html")
-  await page.waitForSelector("#frame_preload_anchor")
-  await nextBeat()
+  await nextEventOnTarget(page, "hello", "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("#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 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
@@ -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)