Guard [data-turbo-preload] with conditionals (#1033)

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.

Co-authored-by: Julian Rubisch <julian@julianrubisch.at>

src/core/drive/preloader.js

@@ -4,27 +4,28 @@ import { fetch } from "../../http/fetch"
 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)
+      }
     }
   }
 
@@ -45,4 +46,8 @@ export class Preloader {
       // If we cannot preload that is ok!
     }
   }
+
+  #preloadAll = () => {
+    this.preloadOnLoadLinksForView(document.body)
+  }
 }

src/core/session.js

@@ -21,7 +21,6 @@ import { Cache } from "./cache"
 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)
 
@@ -44,6 +43,7 @@ export class Session {
 
   constructor(recentRequests) {
     this.recentRequests = recentRequests
+    this.preloader = new Preloader(this, this.view.snapshotCache)
   }
 
   start() {
@@ -78,6 +78,7 @@ export class Session {
       this.streamObserver.stop()
       this.frameRedirector.stop()
       this.history.stop()
+      this.preloader.stop()
       this.started = false
     }
   }
@@ -137,6 +138,25 @@ 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 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 {
+      const location = new URL(element.href)
+
+      return this.elementIsNavigatable(element) && locationIsVisitable(location, this.snapshot.rootLocation)
+    }
+  }
+
   // History delegate
 
   historyPoppedToLocationWithRestorationIdentifierAndDirection(location, restorationIdentifier, direction) {

src/tests/fixtures/frame_preloading.html

@@ -8,5 +8,11 @@
   </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>
+
+    <a href="/src/tests/fixtures/frames/hello.html" data-turbo-frame="hello" data-turbo-preload>Navigate #hello from the outside</a>
   </body>
 </html>

src/tests/fixtures/frames/preloading.html

@@ -8,8 +8,7 @@
   </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>
     </turbo-frame>
   </body>
 </html>

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>

src/tests/functional/preloader_tests.js

@@ -1,53 +1,88 @@
 import { test } from "@playwright/test"
 import { assert } from "chai"
-import { nextBeat } from "../helpers/page"
+import { nextEventOnTarget } from "../helpers/page"
 
 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(() => {
-      const preloadedUrl = "http://localhost:9000/src/tests/fixtures/preloaded.html"
-      const cache = window.Turbo.session.preloader.snapshotCache.snapshots
+  const preloadLink = await page.locator("#preload_anchor")
+  const href = await preloadLink.evaluate((link) => link.href)
 
-      return preloadedUrl in cache
-    })
-  )
+  assert.ok(await urlInSnapshotCache(page, href))
 })
 
 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(() => {
-      const preloadedUrl = "http://localhost:9000/src/tests/fixtures/preloaded.html"
-      const cache = window.Turbo.session.preloader.snapshotCache.snapshots
+  const preloadLink = await page.locator("#preload_anchor")
+  const href = await preloadLink.evaluate((link) => link.href)
 
-      return preloadedUrl in cache
-    })
-  )
+  assert.ok(await urlInSnapshotCache(page, href))
 })
 
-test("navigates to preloaded snapshot from frame", async ({ page }) => {
-  // contains `a[rel="preload"][href="http://localhost:9000/src/tests/fixtures/preloaded.html"]`
+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("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("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)
+
+  assert.notOk(await urlInSnapshotCache(page, href))
+})
+
+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(() => {
-      const preloadedUrl = "http://localhost:9000/src/tests/fixtures/preloaded.html"
-      const cache = window.Turbo.session.preloader.snapshotCache.snapshots
+  const link = await page.locator("a[data-turbo-frame=hello]")
+  const href = await link.evaluate((link) => link.href)
+
+  assert.notOk(await urlInSnapshotCache(page, href))
+})
+
+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 preloadedUrl in cache
-    })
-  )
+  assert.notOk(await urlInSnapshotCache(page, href))
 })
+
+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)
+}

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)