Add support for `data-turbo-replace-method="morph"` to force a morph #1145

krschacht · 2024-01-29T22:19:28Z

This PR allows you to add an optional data-turbo-replace-method="morph" in the cases where a data-turbo-action="replace". If you try to specify morph when the action is advance 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 a replace action (not advance) 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 within navigation_tests.js, specifically:

Within a href links
Within <form> tags (get & post)
Within the <button> tag within a <form> (get & post)

morki · 2024-01-30T09:55:50Z

Another use case is saving new entity and continuing editation, like saving draft, e.g.:

/posts/new -> save draft -> /posts/{id}

It is other URL, but it is the same form and template.

krschacht · 2024-01-30T14:12:26Z

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.

morki · 2024-02-01T17:04:25Z

@krschacht sorry, I never touched the codebase, I am primarily backend developer, so I am pure user of this library :)

pch · 2024-02-12T10:24:19Z

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 refresh accurately describes what's happening here. Maybe a new type of turbo-action is needed.

pfeiffer · 2024-02-15T09:15:54Z

In my opinion, using a refresh action to fetch a URL that is different than the current feels somewhat off.

I think the replace visit action is the appropriate one here. Would it be an option to provide a granular data attribute to control how the replacing would happen instead?

<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>

krschacht · 2024-02-19T14:55:38Z

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.

pfeiffer · 2024-02-26T12:20:11Z

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 refresh to a URL that's different than the current URL - it's not really a refresh then, right? Semantically, a refresh should be of the of the current page and while it could be the same page even with different params (ie &utm_...) it could also not be (eg. ?tab=overview) - it's implementation specific and could be different for every application.

To cover the use-case that @brunoprietog outlines in #1079, I would think that a replace with morphing like this provided by PR would actually be a better solution than comparing pathnames as in #1079 to determine if two URLs could be morphed.

@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 replace with morph enabled cover that use case?

brunoprietog · 2024-02-26T14:38:22Z

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.

pfeiffer · 2024-02-26T14:52:58Z

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.

@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 replace and morph (as this PR would add) or a Turbo Frame with replace.

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?

brunoprietog · 2024-02-26T15:06:53Z

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.

pfeiffer · 2024-02-26T15:09:27Z

@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?

brunoprietog · 2024-02-26T15:13:30Z

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.

pfeiffer · 2024-02-26T15:22:43Z

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!").

krschacht · 2024-02-26T15:26:52Z

@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:

The case where you're on an /post/new and you save mid-stream but you want to continue editing so after the save the user is redirected to /post/1/edit. This is a case where action=replace and replace-method=morph would work.
Think about pagination. Often it's implemented as "/comments?page=1". I believe Turbo already handles this because the querystring is ignored and so the URL can be considered a match (i.e. a refresh). Am I correct about that? But there may be cases where you implement pagination without the query string. Imagine a case where you're paging through full photos, one per page. The URL may go from: /photo/1 to /photo/2. It can be really nice to morph because there may be a bunch of other state on the page that you want to remain and the actual HTML replacement is really minimal.

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.

brunoprietog · 2024-02-26T15:27:48Z

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?

krschacht · 2024-02-26T15:30:19Z

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 data-turbo-action="replace" data-turbo-replace-method="morph" makes more sense conceptually.

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 replace method or if we want to declare morph even for advance. So last night I started thinking through a variation which makes this independent of the replace action.

I'd propose that we allow the method to be declared explicitly for either replace or advance. Meaning:

data-turbo-action="replace | advance" data-turbo-action-method="morph | body"

For a href, the default action is advance and the default method would body. With this change, we could also revert #1079, since that is automatic behavior. We make the automatic behavior only kick in when the full href matches (including matching query params) and then give people the ability to explicitly set the action-method=morph.

pfeiffer · 2024-02-26T15:41:06Z

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?

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.

krschacht · 2024-02-26T15:51:20Z

@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?

pfeiffer · 2024-02-26T15:59:33Z

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 data-turbo-action="replace" data-turbo-replace-method="morph" makes more sense conceptually.

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.

Exactly. Conceptually, a refresh would be a "request to fetch the updated state of this resource". If the constraints change (think ?page=.. or ?coupon=..) I would argue that it is indeed a replace. I do think that it'd be useful to be able to morph the replace action as suggested in this PR, though!

However, as I began implementing this and as I consider the pagination case (my 2nd example in my recent comment) then I started wondering if it's only a replace method or if we want to declare morph even for advance. So last night I started thinking through a variation which makes this independent of the replace action.

There has been discussions previously regarding this.

I can imagine there'd be some issues with this, namely around restore - are they also supposed to morph 'back'?

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 id and [data-turbo-morph] in which case the custom renderer morphs them after replace to switch the bodies.

pfeiffer · 2024-02-26T16:01:08Z

Added the renderer as a gist here in case it's of any interest: https://gist.github.com/pfeiffer/54e3bf929637cad42d534c84f65b1e99

krschacht · 2024-02-26T16:19:07Z

@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 advance with morph is a full screen Next-pagination from /photos/1 to /photos/2

brunoprietog · 2024-02-26T16:37:04Z

@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 href instead of pathname is only given by a limitation in the handling of those adapters. Wouldn't this solution have a similar problem in Turbo native?

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 href for the review would be a step backwards in terms of how magical it is.

Maybe @jorgemanrubia or @afcapel would have a stronger opinion on this.

pfeiffer · 2024-02-26T16:37:29Z

@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.

Yeah, it's vanilla Turbo. We listen to the turbo:before-render which allows you to provide your own render method. I've updated the gist to include how we do that: https://gist.github.com/pfeiffer/54e3bf929637cad42d534c84f65b1e99#file-turbo_render-js

pfeiffer · 2024-02-26T16:47:29Z

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 href instead of pathname is only given by a limitation in the handling of those adapters.

Yeah, well - sort of. The native adapters revealed a potential 'misuse' of the refresh concept to perform URL changes with morphing. The use-case in #1079 and #1145 (comment) is absolutely valid, but I think it would be better solved by something like the suggested change in this PR, reserving the refresh to the action of getting the latest state of the current visited URL, not changing URLs with morphing.

Besides, the current behavior of assuming a replace visit with similar pathname is a page refresh with morphing, makes it really hard to opt-out of the morphing - imagine you'd like to not morph a replace visit from /new to /new?something but still allow both of these to individually be refreshed eg. via broadcasts. You'd need to inspect the request.referer and output a meta tag disallowing morphing based on some params and a referer, which feels off.

brunoprietog · 2024-02-26T17:04:53Z

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.

krschacht · 2024-02-26T19:23:27Z

src/tests/fixtures/destination_for_morphing.html

+    <turbo-frame id="navigate-top">
+      Replaced only the frame
+    </turbo-frame>
+  </body>


TODO: I can probably simplify this file a bit.

krschacht · 2024-02-26T19:25:43Z

src/tests/server.mjs

@@ -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)


While I was removing all my console.log() statements I found this old one left in the code.

src/core/drive/page_view.js

krschacht · 2024-02-26T19:35:29Z

@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:
https://github.com/hotwired/turbo/pull/1145/files#diff-ca04d88186bdf2d24a3a6a146366aac4be3561b4978380ee05066ce7d6ec70caR115

Regarding your comment:

Meta tags are supposed to determine whether morphing should be done or not.

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 <meta name="turbo-refresh-method" content="morph">, and then you create a page which does home.php?page=1 which does a replace link to home.php?page=2. You're surprised because you wanted to do a replace but you did not expect that to morph, since it's not a refresh — it's a navigation to a conceptually different URL and content. This is the problem with assuming that a replace visit with a similar pathname is a refresh.

With this PR you now explicitly specify that the replace-method should be morph. It does not automatically decide it for you.

pfeiffer · 2024-02-26T19:39:26Z

src/core/drive/navigator.js

+  #getReplaceMethodForFormSubmission(formSubmission, fetchResponse) {
+    if (this.#getActionForFormSubmission(formSubmission, fetchResponse) !== "replace") return
+    const { submitter, formElement } = formSubmission
+    return getVisitReplaceMethod(submitter, formElement) || "body"


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.

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.

I have no strong opinions of this; my initial thought was replace, replaceBody or default?

src/core/drive/page_view.js

brunoprietog · 2024-02-26T19:55:39Z

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 href or pathname. But this remains to be verified.

Personally I like where this is headed.

jorgemanrubia · 2024-02-27T08:30:23Z

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 data-* attributes to set the replacement methods are the way to go here. It's not just about offering a solution for the problem at hand but about Turbo as a whole and its programming model. Every new option we add adds complexity, and we need to be very mindful about this, because Turbo offers quite a few options already.

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.

krschacht · 2024-02-27T15:38:06Z

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 pathname comparison.

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. :)

pfeiffer · 2024-02-27T16:07:26Z

@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 replace visit with equal pathname's (and not hrefs) as morphable refresh feels off and this PR provided a way to rollback that change while still allowing control over if a replace action should be morphable.

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)).

krschacht mentioned this pull request Jan 29, 2024

Page refreshes: Use pathname instead of href to detect if locations are equal #1079

Merged

websymphony mentioned this pull request Feb 9, 2024

Allow morphing between different URLs #1179

Closed

pfeiffer mentioned this pull request Feb 15, 2024

Introduce refresh visit action and method to refresh the current page #1186

Closed

krschacht added 2 commits February 26, 2024 11:37

Added new navigation tests which fail

534474d

First two tests passing (regular link)

31b10f0

krschacht added 2 commits February 26, 2024 12:41

Get next eight tests passing (form get & post)

6f962bd

Remove & add some comments

4b1f3f8

krschacht force-pushed the add-refresh-data-turbo-action branch from bff8075 to 4b1f3f8 Compare February 26, 2024 18:55

krschacht changed the title ~~Add support for data-turbo-action="refresh" to force a morph~~ Add support for data-turbo-replace-method="morph" to force a morph Feb 26, 2024

Revert behavior of hotwired#1079

48b520a

krschacht force-pushed the add-refresh-data-turbo-action branch from cda7f16 to 48b520a Compare February 26, 2024 19:20

krschacht commented Feb 26, 2024

View reviewed changes

pfeiffer reviewed Feb 26, 2024

View reviewed changes

src/core/drive/page_view.js Outdated Show resolved Hide resolved

pfeiffer reviewed Feb 26, 2024

View reviewed changes

src/core/drive/page_view.js Outdated Show resolved Hide resolved

Improve readability of the logic

92807a9

krschacht closed this Feb 27, 2024

seanpdoyle mentioned this pull request Sep 24, 2024

Expose morphing functions for consumer use #1319

Draft

Add support for data-turbo-replace-method="morph" to force a morph #1145

Add support for data-turbo-replace-method="morph" to force a morph #1145

Conversation

krschacht commented Jan 29, 2024 • edited Loading

morki commented Jan 30, 2024

krschacht commented Jan 30, 2024

morki commented Feb 1, 2024

pch commented Feb 12, 2024

pfeiffer commented Feb 15, 2024

krschacht commented Feb 19, 2024

pfeiffer commented Feb 26, 2024

brunoprietog commented Feb 26, 2024

pfeiffer commented Feb 26, 2024

brunoprietog commented Feb 26, 2024

pfeiffer commented Feb 26, 2024

brunoprietog commented Feb 26, 2024

pfeiffer commented Feb 26, 2024 • edited Loading

krschacht commented Feb 26, 2024 • edited Loading

brunoprietog commented Feb 26, 2024

krschacht commented Feb 26, 2024 • edited Loading

pfeiffer commented Feb 26, 2024 • edited Loading

krschacht commented Feb 26, 2024 • edited Loading

pfeiffer commented Feb 26, 2024

pfeiffer commented Feb 26, 2024

krschacht commented Feb 26, 2024

brunoprietog commented Feb 26, 2024

pfeiffer commented Feb 26, 2024

pfeiffer commented Feb 26, 2024 • edited Loading

brunoprietog commented Feb 26, 2024

krschacht Feb 26, 2024

Choose a reason for hiding this comment

krschacht Feb 26, 2024

Choose a reason for hiding this comment

krschacht commented Feb 26, 2024 • edited Loading

pfeiffer Feb 26, 2024

Choose a reason for hiding this comment

krschacht Feb 26, 2024

Choose a reason for hiding this comment

pfeiffer Feb 26, 2024

Choose a reason for hiding this comment

brunoprietog commented Feb 26, 2024

jorgemanrubia commented Feb 27, 2024

krschacht commented Feb 27, 2024

pfeiffer commented Feb 27, 2024

Add support for `data-turbo-replace-method="morph"` to force a morph #1145

Add support for `data-turbo-replace-method="morph"` to force a morph #1145

krschacht commented Jan 29, 2024 •

edited

Loading

pfeiffer commented Feb 26, 2024 •

edited

Loading

krschacht commented Feb 26, 2024 •

edited

Loading

krschacht commented Feb 26, 2024 •

edited

Loading

pfeiffer commented Feb 26, 2024 •

edited

Loading

krschacht commented Feb 26, 2024 •

edited

Loading

pfeiffer commented Feb 26, 2024 •

edited

Loading

krschacht commented Feb 26, 2024 •

edited

Loading