Fix double fetch requests (render) for eager-loaded frames

[blrB]

Closes #515 after 256418f with resolves the broken assertion issues in frame_test.ts (fixed by @seanpdoyle).

This decision use 'willRender' option false for the first action ("advance") when the second action ("replace") is created to prevent double rendering frames because when it is rendered two times and read 'src' attributes two times, it makes two fetch request for this frames.

I did not find a solution with 'turbo-frame[complite]' attribute because it still has a value 'false' before the second fetch request.

The previous decision (#516) used 'willRender' option false for the second act, and it has the bug, that was resolved in #674

After this MR next failed tests will be a success:

    [chrome] › frame_tests.ts:830:1 › test navigating a eager frame with a link[method=get] that does not fetch eager frame twice
    [firefox] › frame_tests.ts:830:1 › test navigating a eager frame with a link[method=get] that does not fetch eager frame twice

This issue can be reproduced on test fixtures page (http://localhost:9000/src/tests/fixtures/frames.html), after clicking the link "Eager-loaded frame after GET redirect":

Before:

After:

[seanpdoyle]

If this were declared as a property, it could be invoked without as property access instead of a function call (that is, without the ()):

get willRedirectAsReplaceAction() {
  return this.redirectedToLocation && !this.followedRedirect && this.response?.redirected
}

// elsewhere
this.willRedirectAsReplaceAction

[blrB]

Thank you for comment. I will change this lines.

[blrB]

Done in 9ac687a. Thank you.

[seanpdoyle]

@blrB this has drifted out of date with the latest development from main.

I've rebased this locally, and this is the following diff:

diff --git a/src/core/drive/visit.ts b/src/core/drive/visit.ts
index 6028f57..0274b39 100644
--- a/src/core/drive/visit.ts
+++ b/src/core/drive/visit.ts
@@ -164,6 +164,10 @@ export class Visit implements FetchRequestDelegate {
     return this.isSamePage
   }
 
+  get willRedirectAsReplaceAction() {
+    return this.redirectedToLocation && !this.followedRedirect && this.response?.redirected
+  }
+
   start() {
     if (this.state == VisitState.initialized) {
       this.recordTimingMetric(TimingMetric.visitStart)
@@ -258,7 +262,8 @@ export class Visit implements FetchRequestDelegate {
         if (this.shouldCacheSnapshot) this.cacheSnapshot()
         if (this.view.renderPromise) await this.view.renderPromise
         if (isSuccessful(statusCode) && responseHTML != null) {
-          await this.view.renderPage(PageSnapshot.fromHTMLString(responseHTML), false, this.willRender, this)
+          const willRender = this.willRedirectAsReplaceAction ? false : this.willRender
+          await this.view.renderPage(PageSnapshot.fromHTMLString(responseHTML), false, willRender, this)
           this.performScroll()
           this.adapter.visitRendered(this)
           this.complete()
@@ -313,8 +318,8 @@ export class Visit implements FetchRequestDelegate {
   }
 
   followRedirect() {
-    if (this.redirectedToLocation && !this.followedRedirect && this.response?.redirected) {
-      this.adapter.visitProposedToLocation(this.redirectedToLocation, {
+    if (this.willRedirectAsReplaceAction) {
+      this.adapter.visitProposedToLocation(this.redirectedToLocation!, {
         action: "replace",
         response: this.response,
         shouldCacheSnapshot: false,
diff --git a/src/tests/functional/frame_tests.ts b/src/tests/functional/frame_tests.ts
index dbb90d3..22abfd6 100644
--- a/src/tests/functional/frame_tests.ts
+++ b/src/tests/functional/frame_tests.ts
@@ -44,13 +44,16 @@ test("test navigating a frame with Turbo.visit", async ({ page }) => {
   await page.evaluate((pathname) => window.Turbo.visit(pathname, { frame: "frame" }), pathname)
   await nextBeat()
 
-  assert.equal(await page.textContent("#frame h2"), "Frames: #frame", "does not navigate a disabled frame")
+  assert.equalIgnoringWhitespace(
+    await page.textContent("#frame h2"),
+    "Frames: #frame", "does not navigate a disabled frame"
+  )
 
   await page.locator("#frame").evaluate((frame) => frame.removeAttribute("disabled"))
   await page.evaluate((pathname) => window.Turbo.visit(pathname, { frame: "frame" }), pathname)
   await nextBeat()
 
-  assert.equal(await page.textContent("#frame h2"), "Frame: Loaded", "navigates the target frame")
+  assert.equalIgnoringWhitespace(await page.textContent("#frame h2"), "Frame: Loaded", "navigates the target frame")
 })
 
 test("test navigating a frame a second time does not leak event listeners", async ({ page }) => {
@@ -232,7 +235,8 @@ test("test the turbo:frame-missing event following a link to a page without a ma
   await nextEventNamed(page, "turbo:load")
 
   assert.equal(pathname(page.url()), "/src/tests/fixtures/frames.html")
-  assert.ok(await hasSelector(page, "#missing-frame-link"))
+  assert.ok(await hasSelector(page, "#missing #missing-frame-link"))
+  assert.ok(await hasSelector(page, "#missing #missing-page-link"))
 })
 
 test("test following a link to a page with a matching frame does not dispatch a turbo:frame-missing event", async ({
@@ -281,8 +285,8 @@ test("test following a link within a descendant frame whose ancestor declares a
   const frame = await page.textContent("#frame h2")
   const nestedRoot = await page.textContent("#nested-root h2")
   const nestedChild = await page.textContent("#nested-child")
-  assert.equal(frame, "Frames: #frame")
-  assert.equal(nestedRoot, "Frames: #nested-root")
+  assert.equalIgnoringWhitespace(frame, "Frames: #frame")
+  assert.equalIgnoringWhitespace(nestedRoot, "Frames: #nested-root")
   assert.equalIgnoringWhitespace(nestedChild, "Frame: Loaded")
   assert.equal(await attributeForSelector(page, "#frame", "src"), null)
   assert.equal(await attributeForSelector(page, "#nested-root", "src"), null)
@@ -346,7 +350,7 @@ test("test following a link that declares data-turbo-frame='_self' within a fram
   await nextBeat()
 
   const title = await page.textContent("body > h1")
-  assert.equal(title, "Frames")
+  assert.equalIgnoringWhitespace(title, "Frames")
   assert.ok(await hasSelector(page, "#navigate-top"))
   const frame = await page.textContent("#navigate-top")
   assert.equalIgnoringWhitespace(frame, "Replaced only the frame")

In the time since you've opened this PR, #843 has merged to resolve a lot of the same underlying test harness issues that you've resolved on this branch with trim(...) calls.

Could you try rebasing yourself, then pushing up a commit with the resolved conflicts?

[seanpdoyle]

@blrB with the diff mentioned in #779 (comment), I'm curious if there are tests that cover the original buggy behavior. With the replacement of assert.equal(trim(...)) with assert.equalIgnoringWhitespace, I'm having trouble spotting the new test coverage.

[blrB]

@seanpdoyle

a lot of the same underlying test harness issues that you've resolved on this branch with trim(...) calls.

Hi. This branch includes your commit with the trim method: https://github.com/hotwired/turbo/compare/seanpdoyle:frame-test-broken-assertion

This commit make test ("test navigating a eager frame with a link[method=get] that does not fetch eager frame twice") failed: https://github.com/hotwired/turbo/blob/main/src/tests/functional/frame_tests.ts#L848 due to assert.equal(fetchLogs.length, 1) not worked, please see #515 (comment).

[blrB]

seanpdoyle

Issue doesn't reproduce on the main branch. Because you already returned my code willRender: false from #516 in #804, that was deleted by you in #674. I will close this pull request and close issue #515. Thank you!