Double fetch requests for eager-loaded frames

[blrB]

I think I found a bug.

Steps to reproduce:

1. Create page with eager-loaded frame
2. Create link with redirect on this page (HTTP code not important)
3. Click on link for get redirect
4. Watch browser network tab (or sever logs)

Example requests for turbo_frame with id: src_content:
Chrome:

Firefox:

All my 'eager-loaded frames' were loaded twice. You can look part of my source code for ruby 3.1.0, rails 7.0.1, turbo-rails 1.0.1 here: https://gist.github.com/blrB/1b0e1d44f31df1d1c9932985471ec3b3

I think, that root case can be in line https://github.com/hotwired/turbo/blame/main/src/core/drive/visit.ts#L274 (commit 38b8f13). Now redirect responses use visit action replace. This visit create next trace start -> ... -> loadResponse -> render -> ... -> replaceBody -> sourceURLChanged -> loadSourceURL -> fetch request for frame. But fetch request already was created and rendered, before replace action had been created. I think we shouldn't render this action twice, so we can just create this action with prams willRender: false. I tested this code, and I think it works like I expected. You can look source code here: #516

[john-kelly]

I’m running into this as I’m upgrading from turbolinks.

As a question for the maintainers of the project, is this intended/expected behavior? I imagine others have run into this and I couldn’t find any official statements or documentation regarding this behavior.

My rough understanding is that things changed once turbo started using fetch - and therefore could handle redirects.

Thanks so much for documenting this issue @blrB. And thank you to the turbo maintainers.

[dhh]

Going off memory here, I think the issue is that the snapshot that's showed first before the page is fetched again triggers the frame loading. Thus you end up with the double loads. If that's still the case, then it seems like we need to figure out how to make the snapshots permanent with the frames all loaded. I think maybe @seanpdoyle have had a look at this in the past?

[seanpdoyle]

I believe a change like the one made in #487 might help resolve this issue. It proposes a [loaded] attribute (there are some additional considerations regarding the name), which would expose a frame's load state in a cache-friendly way.

[blrB]

Hello. I try use turbo-rails (1.3.1), but this issue still presents. I can't understand how #674 resolve this issue. I see that frame has complete attribute, but it is still make two fetch request.

@seanpdoyle, could you please help me with this?

[blrB]

Also this issue can be reproduce in test fixtures page (http://localhost:9000/src/tests/fixtures/frames.html), after click link "Eager-loaded frame after GET redirect":

Unfortunately, the written tests did not fail during the regression run(((

line this.assert.equal(fetchLogs.length, 1) for test "test navigating a eager frame with a link[method=get] that does not fetch eager frame twice" works incorrect, due to fetchLogs variable has two elements and test should be failed:

[
  [
    'turbo:before-fetch-request',
    {
      fetchOptions: [Object],
      url: 'http://localhost:9000/src/tests/fixtures/frames/frame_for_eager.html',
      resume: undefined
    },
    'eager-loaded-frame'
  ],
  [
    'turbo:before-fetch-request',
    {
      fetchOptions: [Object],
      url: 'http://localhost:9000/src/tests/fixtures/frames/frame_for_eager.html',
      resume: undefined
    },
    'eager-loaded-frame'
  

Instead of this, test marks as passed. It is strange. It is look like assert.equal() method not works at all.

[seanpdoyle]

@blrB thank you for continuing to pull on this thread.

Unfortunately, the written tests did not fail during the regression run

I've cut a branch that resolves the broken assertion issues, and causes the test you wrote to fail:

https://github.com/hotwired/turbo/compare/seanpdoyle:frame-test-broken-assertion

The willRender: false option you introduced in #516 was eventually reverted by 256418f, but the silently failing test did not catch the regression.

@dhh could you re-open this issue?

@blrB are you interested in continuing to experiment with a solution?

[blrB]

@seanpdoyle thank you for response

are you interested in continuing to experiment with a solution?

Yes, I try to check your alternative option described in 256418f on next week.

[blrB]

#779 (comment)

Issue doesn't reproduce on the main branch.