Skip Snapshot Caching for redirect visits

seanpdoyle · seanpdoyle · commit 903d552a76fb · 2022-11-25T14:05:15.000-05:00
Closes [hotwired#794][] Reverts the implementation change made in [hotwired#674][], and in its place passes `shouldCacheSnapshot: false` alongside `willRender: false` for the `action: "replace"` Visit proposed when following a redirect. In order to test this behavior, this commit introduces the `readBodyMutationLogs` test utility function, along with the `window.bodyMutationLogs` property and the `BodyMutationLog` type. `BodyMutationLog` instances are pushed onto the log `Array` whenever the `<body>` element is replaced. [hotwired#794]: hotwired#794 [hotwired#674]: hotwired#674
diff --git a/src/core/drive/visit.ts b/src/core/drive/visit.ts
@@ -317,6 +317,8 @@ export class Visit implements FetchRequestDelegate {
       this.adapter.visitProposedToLocation(this.redirectedToLocation, {
         action: "replace",
         response: this.response,
+        shouldCacheSnapshot: false,
+        willRender: false,
       })
       this.followedRedirect = true
     }
diff --git a/src/tests/fixtures/rendering.html b/src/tests/fixtures/rendering.html
@@ -45,6 +45,7 @@ <h1>Rendering</h1>
       <p><a id="permanent-in-frame-element-link" href="/src/tests/fixtures/permanent_element.html" data-turbo-frame="frame">Permanent element in frame</a></p>
       <p><a id="permanent-in-frame-without-layout-element-link" href="/src/tests/fixtures/frames/without_layout.html" data-turbo-frame="frame">Permanent element in frame without layout</a></p>
       <p><a id="delayed-link" href="/__turbo/delayed_response">Delayed link</a></p>
+      <p><a id="redirect-link" href="/__turbo/redirect">Redirect link</a></p>
       <form>
         <input type="text" id="text-input">
         <input type="radio" id="radio-input">
diff --git a/src/tests/fixtures/test.js b/src/tests/fixtures/test.js
@@ -46,6 +46,17 @@
        }
      }
    }).observe(document, { subtree: true, childList: true, attributes: true })
+
+  window.bodyMutationLogs = []
+  addEventListener("turbo:load", () => {
+    new MutationObserver((mutations) => {
+      for (const { addedNodes } of mutations) {
+        for (const { localName, outerHTML } of addedNodes) {
+          if (localName == "body") bodyMutationLogs.push([outerHTML])
+        }
+      }
+    }).observe(document.documentElement, { childList: true })
+  }, { once: true })
 })([
   "turbo:click",
   "turbo:before-stream-render",
diff --git a/src/tests/functional/rendering_tests.ts b/src/tests/functional/rendering_tests.ts
@@ -6,7 +6,9 @@ import {
   isScrolledToTop,
   nextBeat,
   nextBody,
+  nextBodyMutation,
   nextEventNamed,
+  noNextBodyMutation,
   pathname,
   propertyForSelector,
   readEventLogs,
@@ -538,6 +540,13 @@ test("test error pages", async ({ page }) => {
   assert.equal(await page.textContent("body"), "404 Not Found: /nonexistent\n")
 })
 
+test("test rendering a redirect response replaces the body once and only once", async ({ page }) => {
+  await page.click("#redirect-link")
+  await nextBodyMutation(page)
+
+  assert.ok(await noNextBodyMutation(page), "replaces <body> element once")
+})
+
 function deepElementsEqual(
   page: Page,
   left: JSHandle<SVGElement | HTMLElement>[],
diff --git a/src/tests/helpers/page.ts b/src/tests/helpers/page.ts
@@ -10,6 +10,9 @@ type MutationAttributeName = string
 type MutationAttributeValue = string | null
 type MutationLog = [MutationAttributeName, Target, MutationAttributeValue]
 
+type BodyHTML = string
+type BodyMutationLog = [BodyHTML]
+
 export function attributeForSelector(page: Page, selector: string, attributeName: string): Promise<string | null> {
   return page.locator(selector).getAttribute(attributeName)
 }
@@ -112,6 +115,19 @@ export async function listenForEventOnTarget(page: Page, elementId: string, even
   }, eventName)
 }
 
+export async function nextBodyMutation(page: Page): Promise<string | null> {
+  let record: BodyMutationLog | undefined
+  while (!record) {
+    ;[record] = await readBodyMutationLogs(page, 1)
+  }
+  return record[0]
+}
+
+export async function noNextBodyMutation(page: Page): Promise<boolean> {
+  const records = await readBodyMutationLogs(page, 1)
+  return !records.some((record) => !!record)
+}
+
 export async function nextAttributeMutationNamed(
   page: Page,
   elementId: string,
@@ -174,6 +190,10 @@ async function readArray<T>(page: Page, identifier: string, length?: number): Pr
   )
 }
 
+export function readBodyMutationLogs(page: Page, length?: number): Promise<BodyMutationLog[]> {
+  return readArray<BodyMutationLog>(page, "bodyMutationLogs", length)
+}
+
 export function readEventLogs(page: Page, length?: number): Promise<EventLog[]> {
   return readArray<EventLog>(page, "eventLogs", length)
 }

Original file line number	Diff line number	Diff line change
`@@ -317,6 +317,8 @@ export class Visit implements FetchRequestDelegate {`
`317`	`317`	`this.adapter.visitProposedToLocation(this.redirectedToLocation, {`
`318`	`318`	`action: "replace",`
`319`	`319`	`response: this.response,`
	`320`	`+ shouldCacheSnapshot: false,`
	`321`	`+ willRender: false,`
`320`	`322`	`})`
`321`	`323`	`this.followedRedirect = true`
`322`	`324`	`}`