-
Notifications
You must be signed in to change notification settings - Fork 435
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
Add support for data-turbo-replace-method="morph"
to force a morph
#1145
Conversation
Another use case is saving new entity and continuing editation, like saving draft, e.g.:
It is other URL, but it is the same form and template. |
Yes, another good example. Thanks. And @morki, I don't know how well you know the codebase, but I'm new to it. I welcome any feedback you have on the PR. |
@krschacht sorry, I never touched the codebase, I am primarily backend developer, so I am pure user of this library :) |
I created my own proof-of-concept for this: #1179 (issue: #1177) The solution isn't great, but the attached gifs demonstrate the difference morphing makes in this particular case. @krschacht Your version looks much better, although I'm not sure that |
In my opinion, using a I think the <form action="/posts" method="post" data-turbo-action="replace" data-turbo-replace-with="morph">
<input ...>
</form> <a href="/posts/4" data-turbo-action="replace" data-turbo-replace-with="morph">Next post</a> |
Thanks @pfeiffer Great suggestion on the naming. I'm happing to clean up this PR and get it ready for merging in if you're in support of it. I've been waiting for one of the project maintainers to give it a blessing since I know you all have the larger context about what makes sense in this library and what doesn't. |
I'm wondering if this could address some of the issues that are outlined in hotwired/turbo-ios#178 (comment) and #1079 Essentially, I find it a bit off that we can To cover the use-case that @brunoprietog outlines in #1079, I would think that a @krschacht, @brunoprietog - is there something that I've missed in terms of the refresh action to different URLs? When does it make sense to "refresh" a page, that is not same as the current page? Would a |
I feel that both are complementary. Ideally it should be as magical as possible. As I argued in the PR, my way of thinking about it is in terms of a Rails controller. The search params don't generally affect what action each controller invokes, which translates to it being the same page simply with other parameters. And changing one parameter for another shouldn't make much difference to the page, otherwise you'd have another action. Well, this would be changing replace to refresh. I think it's not bad either, but it would ignore the options set in the meta tags. And a new API. In essence I would expect this PR to be merged anyway, and that would imply that it should have support in native adapters as well. |
@brunoprietog - thanks for helping me trying to understand :-) I agree and my concern is that the 'magic' disappears if there's diverging behavior in native adapters and web adapters. I agree that sometimes query params do not matter. Sometimes they are used to filter the same collection type (eg. Rails docs) and sometimes they're simply tracking params. But other times they can drastically change the page (remember Turbo is backend agnostic!) - it depends on the backend implementation. Navigating and 'morphing' between eg. a filtering query param, I believe could be made with a I re-read the original PR and I still have a hard time thinking of a real-life example of when we'd like to refresh to a URL with different query strings? I can imagine we'd like to replace and morph to a URL with different query params and I have a feeling that we might be misusing the refresh action for something that this PR would cover. @brunoprietog Could you help me with a practical example of when a refresh should morph between two URLs that have same path but different query params? How would the visit be triggered? |
Yes, I know it's independent of the backend, but we can't forget how tight the integration between Turbo and Rails is either. Anyway, if you think about it, that's the essence of almost any traditional MVC. Well, the search params are too generic. You could use it to read a search query to filter, to choose whether to highlight or not some element, whether to show or hide something, slightly change the behavior of something, use discount codes in the case of a store, for example, etc. I don't think a Turbo frame is enough. After all, its limitations are what motivated the use of morphing. |
@brunoprietog Could you give me an example of a use-case where a refresh would be used to morph between two URLs that have same path but different query params? How would the visit be triggered? |
Well, you might be on a product checkout page and there is a form to enter a discount code with an apply button. When you press that button, you are redirected to the same path but now you have the code parameter with the discount code. This causes the backend to look for the discount code if there is one and update the prices accordingly. Magically, Turbo did morphing. |
Okay, so in this case, would you be able to use the potential feature in PR like: <form action="/checkouts/new" method="get" data-turbo-action="replace" data-turbo-replace-method="morph">
<input type="text" name="coupon" />
<input type="submit" name="Apply coupon" />
</form> I'm not sure the example you provided is a page "refresh" in it's conceptual meaning, is it? It's revisiting the form, applying the coupon to the page; ie. the content of the page depends on the query param (eg. "You've got 50% off!"). |
@pfeiffer Sorry I'm chiming in late. I actually spent a few hours on this PR yesterday, digging through the code much more deeply and thinking through scenarios. I can share some bigger picture thinking in a sec, but first to directly answer your question:
What's notable about this second case is that you wouldn't want it do it with a replace because you would want the history to keep working. |
Having to do it that way is not so magical anymore. The beauty of page refreshes is just that it's magic, that you almost don't have to think about it. In the example itself, the page is practically the same. Only the prices or a slight message change. Why wouldn't that be an appropriate candidate for a page refresh? |
Bigger picture: I do agree that my original approach of calling something a "refresh" when the URL was changing is bad naming. I think your proposal of However, as I began implementing this and as I consider the pagination case (my 2nd example) then I started wondering if it's only a I'd propose that we allow the method to be declared explicitly for either replace or advance. Meaning:
For a href, the default action is |
Mostly because I think what you're doing in this example is not a refresh - you are not fetching the latest state of the current page shown in the browser and morphing it. Instead, the content differ due to it's constraints - a different query param. Therefor it shouldn't be considered a refresh, but a replace (which could be replaced using morph with this PR) Maybe there's a better example? I have a feeling that the use-case for #1079 would be much better solved with the solution in PR, avoiding the pitfalls of the #1079. |
@brunoprietog I think you are using the word refresh to mean "morphing" whereas Mattias is using the word refresh to mean "the same conceptual page, with an unchanged URL, has an updated state which needs to be retrieved". Mattias and I are proposing an alternate means of telling the system that you want to morph the page, without it needing to be a refresh. That's what this PR is attempting to do. @pfeiffer What do you think of my updated proposal? |
Exactly. Conceptually, a
There has been discussions previously regarding this. I can imagine there'd be some issues with this, namely around As a side note, in our application, we've implemented a custom renderer to support morphing (of certain elements) when doing advance visits. That could also cover the use-case of eg. pagination. We can tag elements with an |
Added the renderer as a gist here in case it's of any interest: https://gist.github.com/pfeiffer/54e3bf929637cad42d534c84f65b1e99 |
@pfeiffer Does your app run on a fork of Turbo in order to support your MorphableRenderer and MorphableSnapshot, or are you able to wire those in as overrides? From your gist I couldn't see how Turbo would be made to use these at the appropriate time. I would like to try this out in my app. I understand there are some issues with restore and morphing. That's probably a good enough reason to restrict morphing only to replace, for now. I can try to re-work this PR to do that. But I do think that we should eventually support cases where you want to advance with a morph and you would, therefore, want to fix the underlying restore / snapshot issue. The reasons for wanting to advance & morph is because (a) the page is changing minimally and there is a bunch of other state that you would like preserved, but (b) the changed state is meaningful enough that you want the URL to update and you want the user to be able to go "back" to the previous version. A good example of wanting |
@krschacht I don't want it to be misunderstood that I'm against this PR, quite the contrary! I also see the need to trigger morphing in other similar scenarios that aren't page refreshes. The discussion with @pfeiffer goes the other way, I feel it is more related to the limitations found in native adapters. I have the feeling that practically the strongest argument for using Anyway, it makes sense to me the point you make about what a page refresh is conceptually. Still, I feel like going back to just using Maybe @jorgemanrubia or @afcapel would have a stronger opinion on this. |
Yeah, it's vanilla Turbo. We listen to the |
Yeah, well - sort of. The native adapters revealed a potential 'misuse' of the Besides, the current behavior of assuming a |
Meta tags are supposed to determine whether morphing should be done or not. This as far as I understand is not being respected in native adapters either. Which makes me wonder even more if we are defining something just because of a limitation in the implementation from the native side or because of something more conceptual. |
bff8075
to
4b1f3f8
Compare
data-turbo-action="refresh"
to force a morphdata-turbo-replace-method="morph"
to force a morph
cda7f16
to
48b520a
Compare
<turbo-frame id="navigate-top"> | ||
Replaced only the frame | ||
</turbo-frame> | ||
</body> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: I can probably simplify this file a bit.
@@ -171,7 +171,6 @@ router.get("/messages", (request, response) => { | |||
function receiveMessage(content, id, target) { | |||
const data = renderSSEData(renderMessage(content, id, target)) | |||
for (const response of streamResponses) { | |||
console.log("delivering message to stream", response.socket?.remotePort) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I was removing all my console.log() statements I found this old one left in the code.
@brunoprietog and @pfeiffer Take a look at this PR now. It's not quite ready to merge in since I need to spend more time examining and testing the cases with turbo-frames, but it covers everything we've been discussing in this thread. I welcome any feedback on it. Bruno — Notably, see the changed behavior from your #1079. Now a querystring change alone will cause a morph not to occur but you can explicitly specify the behavior in order to ensure it does: Regarding your comment:
This is true, but the issue is more subtle than this. The meta tags determine whether refresh should be done with morphing, but there is a separate question of what counts as a refresh. Imagine you're a PHP developer, you're using Turbo, you set With this PR you now explicitly specify that the replace-method should be morph. It does not automatically decide it for you. |
#getReplaceMethodForFormSubmission(formSubmission, fetchResponse) { | ||
if (this.#getActionForFormSubmission(formSubmission, fetchResponse) !== "replace") return | ||
const { submitter, formElement } = formSubmission | ||
return getVisitReplaceMethod(submitter, formElement) || "body" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if there's a better value than body
here. It does not have the same clear action as morph
does. The corresponding renderer is called PageRenderer
and the rendering is not isolated to changing the body; it also does some merging of head
elements etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true that the contents of <head>
also gets merged, but the body is what gets replaced and that's the thing in question; that's what differs between the morph & non-morph case. I was keying off this description from the docs:
During rendering, Turbo Drive replaces the current element outright and merges the contents of the element. The JavaScript window and document objects, and the element, persist from one rendering to the next.
But I initially had this as full
. Do you think that's better? I don't feel strongly so I'm happy to go with full
or something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no strong opinions of this; my initial thought was replace
, replaceBody
or default
?
Good discussion here, these are all good points. What I wanted to say about meta tags is that apparently native adapters are not considering them, so it's not just a matter of Personally I like where this is headed. |
Thanks for the work here @pfeiffer and to everyone for the discussion around it. I understand there are scenarios where fined-grained controls would be useful but not sure that We intentionally left using morphing out of arbitrary navigations because needing those is more rare and to keep the programming model as simple as possible. I know there is a need for this, but I think we need to take a step back to see how to better fit the whole "replacement strategy" idea so that Turbo's story remain as cohesive as possible. |
Hi @jorgemanrubia, no problem. I totally respect that. I'll close this PR; I won't do any further work on it. To be honest, I had already worked around my original need for this with a creative use of . I was only wrapping this up since it seemed like there was a deeper problem it was solving with the Check out my other PR which is ready. I'll tag you in it. And meanwhile, there are a couple other bugs in turbo that are bigger impacts for me so I'll chase those down. :) |
@jorgemanrubia I'm also fine with closing this and agree that it's important to keep things streamlined and simple. My main motivation participating in PR was that the change introduced in #1079, treating a The change in #1079 conceptually is not a refresh (it's a replace to different URLs!) and the inconsistency showed up while implementing morphing in Turbo Native (hotwired/turbo-ios#178 (comment)). |
This PR allows you to add an optional
data-turbo-replace-method="morph"
in the cases where adata-turbo-action="replace"
. If you try to specifymorph
when the action isadvance
it will be ignored. It is an alternate, and more general, solution to #1079 so it also reverts that behavior.Background: After introducing the concept of morphing page refresh, Turbo detects when the current page is being refreshed again in order to decide if it should morph. It determines this by comparing the url of the previous location and the new location. This automatic detection works great, most of the time. However, there are some times when the href may include some tracking query string parameters so the href does not match and a refresh is not triggered. PR #1079 was an attempt to address this.
Problem: However, this discussion pointed out that while query string parameters are sometimes unimportant, other times they completely change the behavior of the page (e.g.
?tab=overview
). In addition, there are some situations where we are doing a replace and that replace is effectively a refresh, even though the URL has changed. Imagine the user is on/posts/new
, clicks "save draft", and redirect to/posts/:id
to continue editing. You designate this as areplace
action (notadvance
) and you want to morph this page since it's the same form and same template.Solution: This PR introduces adds support for
data-turbo-replace-method="morph"
. There are explicit tests for all the relevant cases withinnavigation_tests.js
, specifically:a href
links<form>
tags (get & post)<button>
tag within a<form>
(get & post)