-
Notifications
You must be signed in to change notification settings - Fork 419
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Double fetch requests for eager-loaded frames #515
Comments
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. |
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? |
I believe a change like the one made in #487 might help resolve this issue. It proposes a |
When redirecting to a page that contains elements marked with `[data-turbo-cache="false"]`, those elements are removed _before_ the initial render, instead of _after_ the render and _before_ the page is cached. This behavior seems to have stemmed from [hotwired#516][], which was shipped in response to [hotwired#515][]. As an alternative to the `willRender: false` option passed to `this.adapter.visitProposedToLocation` in `Visit.followRedirect`, the implementation can instead [rely on the presence of the `turbo-frame[complete]`][comment] to guard against double fetching. To guard against regressions, this commit adds coverage for the unwanted behavior by redirecting from `navigation.html` to `cache_observer.html`, and asserting the presence of a `[data-turbo-cache="false"]` element that resembles and application's Flash messaging. [hotwired#515]: hotwired#515 [hotwired#516]: hotwired#516 [comment]: hotwired#515 (comment) [hotwired#487]: hotwired#487
When redirecting to a page that contains elements marked with `[data-turbo-cache="false"]`, those elements are removed _before_ the initial render, instead of _after_ the render and _before_ the page is cached. This behavior seems to have stemmed from [hotwired#516][], which was shipped in response to [hotwired#515][]. As an alternative to the `willRender: false` option passed to `this.adapter.visitProposedToLocation` in `Visit.followRedirect`, the implementation can instead [rely on the presence of the `turbo-frame[complete]`][comment] to guard against double fetching. To guard against regressions, this commit adds coverage for the unwanted behavior by redirecting from `navigation.html` to `cache_observer.html`, and asserting the presence of a `[data-turbo-cache="false"]` element that resembles and application's Flash messaging. [hotwired#515]: hotwired#515 [hotwired#516]: hotwired#516 [comment]: hotwired#515 (comment) [hotwired#487]: hotwired#487
When redirecting to a page that contains elements marked with `[data-turbo-cache="false"]`, those elements are removed _before_ the initial render, instead of _after_ the render and _before_ the page is cached. This behavior seems to have stemmed from [#516][], which was shipped in response to [#515][]. As an alternative to the `willRender: false` option passed to `this.adapter.visitProposedToLocation` in `Visit.followRedirect`, the implementation can instead [rely on the presence of the `turbo-frame[complete]`][comment] to guard against double fetching. To guard against regressions, this commit adds coverage for the unwanted behavior by redirecting from `navigation.html` to `cache_observer.html`, and asserting the presence of a `[data-turbo-cache="false"]` element that resembles and application's Flash messaging. [#515]: #515 [#516]: #516 [comment]: #515 (comment) [#487]: #487
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 @seanpdoyle, could you please help me with this? |
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
Instead of this, test marks as passed. It is strange. It is look like |
@blrB thank you for continuing to pull on this thread.
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 @dhh could you re-open this issue? @blrB are you interested in continuing to experiment with a solution? |
@seanpdoyle thank you for response
Yes, I try to check your alternative option described in 256418f on next week. |
Issue doesn't reproduce on the main branch. |
I think I found a bug.
Steps to reproduce:
Example requests for turbo_frame with id: src_content:
![chrome_duplicate_requests](https://user-images.githubusercontent.com/9513452/150327039-5d657010-f0f9-4bb4-9980-bd5d5054f9b2.png)
![ff_duplicate_requests](https://user-images.githubusercontent.com/9513452/150327045-c3feb0f0-089a-494d-987e-eafe669766a3.png)
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 tracestart
-> ... ->loadResponse
->render
-> ... ->replaceBody
->sourceURLChanged
->loadSourceURL
-> fetch request for frame. But fetch request already was created and rendered, beforereplace
action had been created. I think we shouldn't render this action twice, so we can just create this action with pramswillRender: false
. I tested this code, and I think it works like I expected. You can look source code here: #516The text was updated successfully, but these errors were encountered: