Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Turbo Frame Responses missing tracked elements #580

Open
sfnelson opened this issue Feb 14, 2024 · 6 comments
Open

Turbo Frame Responses missing tracked elements #580

sfnelson opened this issue Feb 14, 2024 · 6 comments

Comments

@sfnelson
Copy link

When rendering a turbo-frame with data-turbo-action="advance" to promote visits, Turbo checks the head of the current document against the head of the incoming response to decide whether to 'render' the synthetic navigation.

In the turbo tests, this check passes because the head of the frame response matches the page response, but in a frame response generated by turbo-rails, the head is empty and so the check fails.

This behaviour changed in Turbo 8/Turbo Rails 2, and as a consequence frame navigation visits are no longer compatible with history restorations.
@sfnelson sfnelson mentioned this issue Feb 14, 2024
Open
sfnelson added a commit to katalyst/kpop that referenced this issue Feb 19, 2024
@sfnelson
Avoid Turbo 8 upgrade until frame navigation is resolved
9e27ac2 
hotwired/turbo-rails#580
@sfnelson sfnelson mentioned this issue Feb 19, 2024
Closed
@seanpdoyle
Copy link
Contributor

Is this also related to hotwired/turbo#1047?

The reproduction in #581 demonstrates that hotwired/turbo#1144 might not've resolved the underlying issue.

@sfnelson
Copy link
Author

sfnelson commented Feb 21, 2024 •
edited
Loading

Yes, I believe that's related. I've found a workaround for the issue – replacing layout/turbo_rails/frame.html.erb and adding the tracked elements to the head element:

<head>
  <%= stylesheet_link_tag "application", "data-turbo-track": "reload" %>
  <%= javascript_importmap_tags %>
  <%= yield head %>
</head>

This change might be acceptable as a short term patch to get frame navigations working with turbo-rails >= 2, but I think it's likely to cause a lot of headaches:

  • Assumes import maps
  • Masks the need to add additional lines for any extra turbo-tracked elements
  • Assumes that any custom layouts use the same elements
    It doesn't seem like a good general solution.

After I made this change, I was running through one of our larger apps updating syntax and I came across a bug that I then spent a good half an hour debugging, only to realise it was this same bug – another layout rendering which doesn't track the application stylesheet – and it was so unclear that this was happening that even though I was literally testing for this change I didn't realise that I was seeing another symptom of this same issue. If the Turbo team is going to stick with the strategy of sending a full <head> in turbo frame responses, then I think there needs to be some mechanism for alerting users when the visit is aborted.

@sfnelson
Copy link
Author

For future me/anyone else debugging this issue, this is what happens when a turbo-frame advance occurs:

  1. Frame navigation triggered
  2. If this is going to be an advance navigation, then proposeVisitIfNavigatedWithAction is called to set up the necessary state tracking and side-effects, this includes taking a whole-page snapshot (saved for later).
  3. XHR request/Frame rendering happens – details are largely irrelevant, this is all normal stuff. After the render finishes the DOM has changed – render is complete and side-effects are visible, but the URL has not changed.
  4. proposeVisitIfNavigatedWithAction's callback fires and calls session.visit.
  5. Session.visit takes the PageVisit path, and calls Navigator.proposeVisit

Session.visit is a somewhat leaky abstraction (e.g. hotwired/turbo#1055). In order to fit the shape of a 'normal' visit the rest of the logic pretends that a full page request has occurred, but the call to visit includes some arguments that control downstream logic and let Turbo know that it doesn't actually need to update the page (e.g. willRender: false) because this was a frame visit.

This is where things start to go wrong:

  1. PageView.renderPage() checks renderer.shouldRender
  2. renderer.shouldRender compares the tracked elements in the heads of the old page and the new page and concludes that they are different
  3. PageView.renderPage() skips calling visit.changeHistory() because it thinks there's going to be a re-render.
  4. However, View.render() skips the call to invalidate because this is a frame navigation (see comment workaround to ignore tracked element mismatch reloads in that method).

The browser history is now inconsistent with the restoration cache. The snapshot for the previous page state never gets saved, but the page doesn't get reloaded because invalidate was never called because this was a frame render. This leaves the developer pretty confused :-)

@sfnelson
Copy link
Author

sfnelson commented Feb 21, 2024 •
edited
Loading

This same bug bit me again, I keep assuming my code is wrong instead of checking that the heads match.

You can tell whether this bug has occurred by checking:

Turbo.session.view.forceReloaded

If it's true after a frame navigation, your history is now invalid.

@lovitt
Copy link

lovitt commented Mar 13, 2024 •
edited
Loading

I spent this morning troubleshooting back button problems in my app, which was recently upgraded to turbo 8.0 / turbo-rails 2.05. I believe the issue outlined here is the culprit. The workaround shared by @sfnelson addresses the problem for me so I will be using that for now.

FWIW, my app isn't doing anything fancy with Turbo, just decorating links with data-turbo-action="advance".

@Rohland
Copy link

Rohland commented Apr 11, 2024 •
edited
Loading

I just upgraded an app to Rails 7.1 and ran into this. Been banging my head against this bug for the last few hours. Glad I tracked down this post 😅 The workaround above worked for me as well (thanks @sfnelson). It doesn't seem ideal, so going to try downgrade turbo-rails for now.

@AntonKhorev AntonKhorev mentioned this issue Sep 20, 2024
Draft
@sfnelson sfnelson mentioned this issue Oct 29, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@lovitt @Rohland @sfnelson @seanpdoyle