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.

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

Breaking change in 7.2, when frame response is 4xx, 5xx OR if frame is missing #841

Closed
cabgfx opened this issue Jan 3, 2023 · 7 comments

Comments

@cabgfx
Copy link

cabgfx commented Jan 3, 2023

On updating from Turbo 7.1.x to to 7.2.x, via turbo-rails, we were quite surprised to see frames suddenly doing full-page redirects on failures, ie. for a 4xx or 5xx response.

I'm aware of #445, and the reasoning behind the turbo:frame-missing event, quite useful!

However, the changes introduced in #693, where Turbo now transforms 4xx and 5xx responses to full-page Visits, feel like a breaking change. The minor version bump from 7.1 to 7.2 certainly broke our app 😅

I've read up on the discussion in #670, and noticed this sentiment by @seanpdoyle :

Scenarios like this make me nervous about breaking the #94 (comment) that a navigation within a frame will stay within a frame.

I believe the changes introduced in 7.2 specifically break that “promise”: a navigation within a frame will stay within a frame

Are apps now required to add additional/custom JS by default, to handle this scenario?

Or am I simply misunderstanding something? (quite possible!)


Related:

@dhh
Copy link
Member

dhh commented Jan 3, 2023

@kevinmcconnell is working on a new feature for 7.3 where we refine this such that only paths from an allowlist, like redirects to /sign_in, will break out of the frame, and other 4xx and 5xx responses will instead stay in the frame, but fill it with an error message. That'll address the original concern where session timeouts would just fail, and the problem with 500s just causing a white screen and no user feedback.

@kevinmcconnell
Copy link
Collaborator

I should have a PR ready for that in the next couple of days.

@cabgfx
Copy link
Author

cabgfx commented Jan 3, 2023

Awesome! Appreciate your work on this! If I can assist with testing it out or anything, please let me know

@duffyjp
Copy link

duffyjp commented Jan 5, 2023

... only paths from an allowlist, like redirects to /sign_in, will break out of the frame, and other 4xx and 5xx responses will instead stay in the frame, but fill it with an error message.

This is a great plan! 🎉

Will it be possible to get the details of what went wrong in the console?

@jon-sully
Copy link

Just for breadcrumbs' sake, the allow-list concept ended up getting scrapped in #864 to instead create a flag (of sorts) to put on the incoming page which forces Turbo to full-visit the page, even if the request originated from within a frame

@jon-sully
Copy link

And with 7.3 I think this issue is closable 😁

@dhh dhh closed this as completed Mar 2, 2023
@cabgfx
Copy link
Author

cabgfx commented Mar 2, 2023

Thanks for the heads-up, @jon-sully!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants