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

turbo:frame-missing: Do not Turbo.visit by default #677

Closed

Conversation

seanpdoyle
Copy link
Contributor

Restore the existing default behavior when a matching frame is missing
from a response: log an error and blank the frame.

Alongside that original behavior, yield the Response instance and a
Turbo.visit-like callback that can transform the Response instance
into a Visit. It also accepts all the arguments that the Turbo.visit
call normally does.

@jon-sully
Copy link

jon-sully commented Aug 18, 2022

Curious why you want to roll this default behavior back, Sean? I know consuming apps could catch the event and do the visit themselves manually (e.g. this could all still work the same way very easily after this PR) but having it as a default feels easier. Looks like there was a little chatter in these comments

EDIT: Ah, I hadn't quite stumbled upon all the issues yet! For others, looks like Sean delved into this one in #670 (comment) and continued conversation is there

Restore the existing default behavior when a matching frame is missing
from a response: log an error and blank the frame.

Alongside that original behavior, yield the [Response][] instance and a
`Turbo.visit`-like callback that can transform the `Response` instance
into a `Visit`. It also accepts all the arguments that the `Turbo.visit`
call normally does.

[Response]: https://developer.mozilla.org/en-US/docs/Web/API/Response
@dhh
Copy link
Member

dhh commented Sep 13, 2022

I remain confident that the fallback to visit on missing frame is the better default. We allow you to catch the frame missing event, so you can always override that default if you see fit.

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

Successfully merging this pull request may close these issues.

3 participants