Skip to content
This repository has been archived by the owner on Jul 28, 2018. It is now read-only.

Partial replacement of new pages #448

Closed
dhh opened this issue Jan 6, 2015 · 30 comments · Fixed by #468
Closed

Partial replacement of new pages #448

dhh opened this issue Jan 6, 2015 · 30 comments · Fixed by #468

Comments

@dhh
Copy link
Contributor

dhh commented Jan 6, 2015

Some great ideas from https://github.com/Shopify/turbograft that we should migrate into Turbolinks. The first is partial replacement. I was initially skeptical to this idea, but have been won over by seeing how it can really work well with permanent elements of a page like shopping carts or sidebars or other pieces with state.

The big win from Turbolinks remain that the initial hit will generate the full page, and that you won't have to tailor server-side rendering on subsequent requests, even when you're only doing partial updates.

I'd like to propose the following API:

<div data-turbolinks-partial="section-a">
</div>

<div data-turbolinks-permanent>
</div>

<a href="#" data-turbolinks-change="section-a"></a>

Turbolinks.visit(url, change: [ 'section-a', 'section-b' ])
Turbolinks.visit(url, keep: [ 'section-c', 'section-d' ])

So you can name a partial with data-turbolinks-partial. These named partials can then be referred to in the Turbolinks.visit call through the change/keep options.

Marking an element with "data-turbolinks-permanent" will mean that it is not updated by any Turbolinks.visit call, except if you ALSO give the same element a partial name AND name that partial as part of a Turbolinks.visit/change call.

@kristianpd
Copy link
Contributor

@dhh interesting to see this discussion taking place. A few comments:

Rouging out a few examples:

<h1>Dashboard</h1>
<nav data-turbolinks-partial="sidebar">
  <a data-turbolinks-partial="order-count">Orders <%= @open_order_count %></a>
</nav>

<h1 data-turbolinks-partial="order-count">You have <%= @open_order_count %> orders that need your attention.</h1>
<%= link_to "Update your account to have more features", onclick="Turbolinks.visit(url, change: ['sidebar'])" %>

<!-- option 1: care less about what changes and more about what doesn't -->
<%= link_to "Capture and fulfill all orders", onclick="Turbolinks.visit(url, keep: ['sidebar'])" %>

<!-- option 2: you have context on what's changing and are likely performing a more focused partial replacement -->
<%= link_to "Capture and fulfill all orders", onclick="Turbolinks.visit(url, change: ['order-count'])" %>

GET vs. POST

A good chunk of Turbograft's design is driven by the fact that you'll be mutating data and then refreshing one or many sections of the page as a result of that change. Turbolinks is different in that it's purely for a faster UX on GET's. For this reason, I'm curious what flow you see the Turbolinks.visit call with change or keep being used in?

  • Is it for when someone performs an AJAX request and then wants to refresh a section of the page they know changed?
  • Is it more to avoid refreshing sections (data-turbolinks-permanent) that will never change?
  • Would it be used in conjunction with remote=true calls to refresh a page after some form of persisting action took place?

change keys can be greedy

In the example above, if you wanted to display an open order count in your sidebar nav, anytime you change the outer key sidebar you will affect any keys within it (at least in our implementation). That's not a problem in this case, but it does raise the question of what you do if you have a data-turbolinks-permanent node inside a node with a change key.

This hasn't caused us any real pain yet, but there have been a few cases where we avoided putting keys on a wrapping node, and became more selective on the child nodes instead because of this.

we try to think of our keys as what-data-changes and not what-sections-change

Since Turbolinks is more GET than POST, I actually may prefer the section-a example of keys if you're thinking of using this more for higher-level layout control than for reflecting data changes in the UI. If however, it's being done more for refreshing sections after mutative calls have taken place, we try to keep our refresh keys more descriptive of the data that changes.

Of note, most of our pages only have 1 or 2 unique change keys on them, with additional refresh-always (sorta like a data-turbolinks-ephemeral) keys on our pages. We also rarely call visit with more than one change key.

a rough thought on partials

A long long time ago, we had the idea that one day we may be able to hook in to rails partials to have speedier page rendering if we introduced the concept of refresh/section keys at a server level. We haven't done this, performance hasn't been an issue and it's probably an over optimization for us however I'm curious what your thoughts are on it playing more with Turbolinks.

The idea was basically

layouts/application.html.erb

  <%= render 'main_nav', "data-turbolinks-partial" => "user_account" %>

  <%= yield %>

views/foos/index.html.erb

  <%= render 'foo_table', "data-turbolink-partial" => "foos" %>
  <%= render 'bar_table', "data-turbolink-partial" => "bars" %>
Turbolinks.visit(url, change: ['foos'])  // only ends up rendering foos partial server side.

If rails would somehow acknowledge (HTTP headers?) change keys, we could make render aware of our partial rendering strategy and simply skip rendering any partials that don't share a key we care about. This was an early idea on how we could cut down on the size of response body but also has a few challenges (like greedy keys mentioned above) that we chose to never overcome. It also proved to not be a performance issue to render an entire layout + body in the back buffer even if we only needed a section or two.

Haven't really thought about this idea in a while, but this PR sparked it again.

Interested in seeing where this goes!

@dhh
Copy link
Contributor Author

dhh commented Jan 6, 2015

Quick note. I think we'd use the name "partial" to refer to a named section, so it'd be:

<h1>Dashboard</h1>
<nav data-turbolinks-partial="sidebar">
  <a data-turbolinks-partial="order-count">Orders <%= @open_order_count %></a>
</nav>

One concern is indeed whether partial is already an overloaded word. I'm starting to think that it probably is. Turbolinks partials wouldn't necessarily correspond fully to Rails partials. So how about data-turbolinks-section="name" instead?

At Basecamp, I see the following use cases currently:

• Keep a navigation sidebar that includes per-user stuff separate from the perma pages for, say, a message. Our sidebar is lazy loaded, but once it's there, page changes shouldn't eject it.
• Server-side responses to updates that use redirect_via_turbolinks_to and use a change/keep selector to mutate just part of the page. Could be when you update subscribers on a post, it only changes that section.
• Every form we use on Basecamp is a remote: true.

I don't think the nested issue is that big of a deal. I'm fine with the fact that if you replace a specific section, any permanent sections within it are overwritten.

Interesting thoughts on the partial element, but at least for us, it's not really an issue. We russian doll cache all our partials. If something hasn't changed, then it won't take material time to render it, because it's coming out of a cache. But it's worth thinking about as a separate element.

@kristianpd
Copy link
Contributor

re: both

So how about data-turbolinks-section="name" instead?

and

Server-side responses to updates that use redirect_via_turbolinks_to and use a change/keep selector to mutate just part of the page. Could be when you update subscribers on a post, it only changes that section.

Agreed that partial is probably a little overloaded at this point. section is pretty good, though my only hesitation there is that section sort of implies you'll have to worry about what segments of page you want to update from the caller instead of letting your DOM + ERB describe the flow of your page.

I'm sensitive to thinking of partial replacements in the space of "what sections need updating on my page", because it feels more SJR to me. This was a big discussion early in our Turbograft days, I had summarized it here. Our end result for a simple add comment flow looks like this with turbograft.

This is getting a little nitpicky but the latter example feels a little more "data driven" to me, which was one big takeaway from our Batman days. Our goal was a single render path on your view that through HTML + ERB describes as much of the flow of your app as possible. In Batman, we had DOM bound to data and the view described the flow, all you had to do was get your data in to the right state.

Perhaps something more like data-turbolinks-watch, which says "I'm interested in being told when this changes". I find this more clear that many elements of the DOM can "watch" the same thing, and you don't get huge chains of change: ['section-a', 'section-b', 'section-c']calls in your JS. This avoids your JS knowing as much about your DOM as your DOM. Your DOM decide what it cares about.

A simple example being:

<nav>
  <a data-turbolinks-watch="comments">Comments (<%= @comments.count %>)</a>
</nav>
<!-- a bunch of other stuff -->
<div data-turbolinks-watch="comments">
  <%= @comments.each do |comment| %> 
    <!-- ... -->
  <% end %>
</div>
<!-- form to add a comment with -->
<!-- Turbolinks.visit(url, change: ['comments']) -->

vs.

<nav>
  <a data-turbolinks-section="comments-nav-counter">Comments (<%= @comments.count %>)</a>
</nav>
<!-- a bunch of other stuff -->
<div data-turbolinks-section="comments-list">
  <%= @comments.each do |comment| %> 
    <!-- ... -->
  <% end %>
</div>
<!-- form to add a comment with -->
<!-- Turbolinks.visit(url, change: ['comments-nav-counter', 'comments-list']) -->

I just see people naturally seeing section as a 1:1 (data:node) relationship instead of a 1:*(data:nodes) where I much prefer the latter.

@dhh
Copy link
Contributor Author

dhh commented Jan 6, 2015

I don't mind the server-side knowing which elements it just updated, and basically passing on optimization hints to the rendering phase. The cool thing about something like this:

def create
  @post.comments.create! comment_params
  redirect_to @post, change: :comments
end

Is that it'll work just as well whether you're in Turbolinks mode or not. So you still have a rendering flow that's completely unaltered regardless of whether it's a first or subsequent load or it's a POST or a GET.

The second thing is that I don't think pages are going to get littered with this. Our initial intention was just to add data-turbolinks-permanent, but even with this more flexible proposal, as you've found, it's usually just 1 or 2 sections that need replacement.

I'm not as big a fan of watch because we're not setting up any event flow here. The elements are not doing the watching, they're the "dumb" part of this. They just have a name that we can refer to in the place that is triggering the action and that does have the knowledge of which parts changed.

@dhh
Copy link
Contributor Author

dhh commented Jan 6, 2015

It's funny, btw, that we're talking about the comment example. I just ripped out some code that basically did $('comments').append('<%=j render @comment %>') in favor of just doing a turbolinks redirect and a "full" replacement. The micro optimization turned out not to be worth it.

I'm finding that in a lot of cases, and I think it'll be even more true when we can do section updates like this. You just won't need as much SJR. You're still doing remote: true forms, but their result will most often just be a Turbolinks.visit response.

@kristianpd
Copy link
Contributor

I'm interested in seeing where we could go with the redirect_to @post, change: :comments, we solved this without a rails change by doing stuff like refresh-on-success. This keeps the refresh keys in the DOM/JS, but perhaps there's a more encompassing Rails approach that relates the loose coupling between keys, ivars and rails conventions.

OK, watch is no better because it definitely sounds like event flows which isn't what it should be. As we built out our entire app, we added helper patterns like refresh-on-success in the DOM because that's where the refresh keys are defined and we were hoping to avoid server side decisions on what keys updated. The simple example being

<a remote="true" href="/orders/1/close" remote-method="POST" refresh-on-success="orders">Close order</a>

This was our solution to common and simple remote: true forms and links. Create would do something like redirect_via_turbolinks_to :index and we'd pull whatever watch/change/partial/section/refresh (:smile:) keys we care about out of the resulting DOM.

The elements aren't watching, but they're hinting that they care about this set of data.

We have no SJR in Shopify with Turbograft and mitigate repetitive Turbolinks.visit type calls with some remote helper attributes like refresh-on-success

@dhh
Copy link
Contributor Author

dhh commented Jan 6, 2015

Yeah, I think that's the part I'm the least hot on. That the dom would basically include conditional logic. To me that's a bridge too far. IMO, the controller knows best how to route this. So it could do:

def create
  if @post.comments.create(comment_params)
    redirect_via_turbolinks_to @post, change: :comments
  else
    redirect_via_turbolinks_to new_post_comment_url(@post), change: :error, error: 'Something not right!'
  end
end

I think flow logic belongs in the controller.

I'm 👍 on cutting down on SJR by having these partial updates that allow state to be retained. I think there's really something here.

@kristianpd
Copy link
Contributor

I'm not quite getting your point on conditional logic in the DOM.

  • We always rely on form helpers/html form tags to build out our action URLs and use data attributes to avoid tons of onSubmit visit calls.
  • In pretty much every case, we use stuff like refresh-on-success to handle our responses. Perhaps this is where you have the issue, and would prefer rails to tell you what's refreshing?
  • We actually have very few visit(change: []) style calls in the end. Once our admin was completed, we had moved most to the few refresh-on-success, refresh-on-error patterns we pulled out. The more I think about it, the more I'm interested in seeing an example where the server defines what to refresh (as you've shown).
  • We originally started out with a POST then separate GET after that response was successful then quickly switched to a POST with inline response when possible to avoid multiple unnecessary XHRs.
  • One nitpick about create style endpoints in your example that you need to render the view inline without a redirect (so we can keep validation errors in context). We end up doing a render action: new and handling that response inline from the create POST (no redirect). This also led us to create a special pattern for modals with form validations (we basically treat it like any other /new endpoint without a layout). This is getting pretty implementation specific so I won't go too far on it here

@dhh
Copy link
Contributor Author

dhh commented Jan 6, 2015

When I say conditional, I mean refresh-on-success/refresh-on-error/full-refresh-on-error-except – that the DOM is specifiying these different paths. In my opinion, it's the responsibility of the controller to direct to what happens after an interaction – especially when it's a multi-path flow, like error handling.

I think the beauty of partial rerendering is exactly that you don't need to do render action: :new, at least not in the Turbolinks case. Since we're not replacing the full form. So you could just do a partial redirect/rerender where you're only changing the error block. But yes, that wouldn't allow for per-field changing of things. But don't think you're getting around that anyway.

@dhh
Copy link
Contributor Author

dhh commented Jan 6, 2015

Anyway, I think you're right that it's not likely that these setups are generally going to be so deeply intricate. Most of the time you're just going to want to either refresh just once specific section, or have one section that you keep around. I think we could start by moving that stuff over, if you're interested? Then we can see what we can possibly build on from there.

I think we absolutely share the same vision, though. Simply re-render whole pages most of the time. A few nibs at stable elements. And this will get us 90% of the way.

@kristianpd
Copy link
Contributor

👍.

One restriction we added to partial replacement nodes is they need an ID as well as the key so we can find the corresponding note in the replacement DOM. There's also a few cases like "what if the node goes away or is created" that need to be handled. Sort of like a poor mans React diff.

I'll try to find a little time this week to open a PR with some partial replacement prototyping.

@dhh
Copy link
Contributor Author

dhh commented Jan 7, 2015

Why can't you find the nodes via ('[data-turbolinks-section=X]')?

And yeah, this def is similar in many ways to the React stuff. Hopefully we can get away with far less complexity.

@kristianpd
Copy link
Contributor

The ('[data-turbolinks-section=x]') selector will only returns nodes that are "watching" that section. As soon as you have multiple nodes "watching" the same section, you can't really make great guesses as to which node you should be replacing in your front buffer DOM with a node from the back buffer DOM. If it's only ever a 1:1 section key, then it would work.

I think we also considered tracking it in a simple index like

front-buffer DOM

{
  0: [div id="customer_list"]
  1: [div id="customer_bubble"]
  2: [div id="customer_modal"]
}

However without some kind of unique identifier (we used id), you run in to problems if your page refresh adds or removes something based on the state change of the data

back-buffer DOM

{
  0: [div id="customer_list"]
  1: [div id="customer_modal"] // how do I know this index 1 is actually index 2 in my front buffer?
}

@dhh
Copy link
Contributor Author

dhh commented Jan 7, 2015

Ah, yes, of course. Makes sense. Although I’d expect that the normal case would be that you only have the 1:1? But I guess we need to deal with both.

On Jan 7, 2015, at 09:57, Kristian PD notifications@github.com wrote:

The ('[data-turbolinks-section=x]') selector will only returns nodes that are "watching" that section. As soon as you have multiple nodes "watching" the same section, you can't really make great guesses as to which node you should be replacing in your front buffer DOM with a node from the back buffer DOM. If it's only ever a 1:1 section key, then it would work.

I think we also considered tracking it in a simple index like

** front-buffer DOM **

{

0: [div id="customer_list"
]

1: [div id="customer_bubble"
]

2: [div id="customer_modal"
]
}

However without some kind of unique identifier (we used id), you run in to problems if your page refresh adds or removes something based on the state change of the data

back-buffer DOM

{

0: [div id="customer_list"
]

1: [div id="customer_modal"] // how do I know this index 1 is actually index 2 in my front buffer?

}


Reply to this email directly or view it on GitHub.

@mrkurt
Copy link

mrkurt commented Jan 20, 2015

This discussion of the future solves a current problem for us. So 👍

@kristianpd
Copy link
Contributor

I'm still planning on getting around to this in a couple weeks, been swamped lately.

I'm going to take a shot at pulling out the partial refresh stuff from Turbograft and hook it up to a respond_with option / controller decision as @dhh had suggested.

@dhh
Copy link
Contributor Author

dhh commented Feb 5, 2015

🤘

On Feb 5, 2015, at 10:10, Kristian PD notifications@github.com wrote:

I'm still planning on getting around to this in a couple weeks, been swamped lately.

I'm going to take a shot at pulling out the partial refresh stuff from Turbograft and hook it up to a respond_with option / controller decision as @dhh had suggested.


Reply to this email directly or view it on GitHub.

@obsidienne
Copy link

With this you can keep an audio/video playing between page navigation so 👍

@aantix
Copy link

aantix commented Feb 9, 2015

I've been following the conversation and have an alternative idea: there's parity between the view's cached fragments and the sections that need to be updated on an XHR request.

Why not have the (view) cache blocks double as the section definitions? When an xhr request is made, the page is rendered but only the cache sections that are re-rendered (non-cached) are sent back to the client (a hash of changes). Some lightweight js can find the prior section and replace it with the new render?

By having a 'section' definition, the developer gets the initial Russian Doll fragment caching along with defining appropriate sections for client side updates.

I'll do a proof of concept and open a separate PR.

@kristianpd
Copy link
Contributor

Yeah, that's a good point @aantix. I had suggested the same to @dhh but the only downside is that now you're coupling the fragment caching to it (maybe not necessarily a downside :)). Two considerations:

  1. You'll need some kind of DOM node hook for the current Turbograft refresh strategy to know what to replace. It's possible for nodes to come and go during a partial replace, so we use id + refresh attributes on the node itself to hook it. You may be able to do something with comment nodes to avoid any possible effect on the DOM structure.
  2. You'll need your cache keys to have some predictable prefix on them as the suffix of the key will likely change with each partial update (the data is changing). You may be able to use action and action_suffix options for this, we don't have this style of caching in our app so I can't really speak to any other gotchas here.

I'm also really interested in seeing if this works, feel free to ping me on PRs. I will likely play around with this at the end of Feb (we have a few hack days coming up), maybe we can bounce a few ideas around before then.

@aantix
Copy link

aantix commented Feb 9, 2015

Great considerations @kristianpd.

  1. For the initial proof of concept, I was going to just render a div. Comment nodes look much more promising and less intrusive. Thanks for the suggestion.

  2. I was hoping div_for was smart enough to create a unique, predictable ID for varying dependencies. It's not the case. I can probably extract something from the cache helper (I think there's a unique ID generated before the cache digest is computed).

I'll definitely ping you on the PR when it's done.

@dhh
Copy link
Contributor Author

dhh commented Feb 10, 2015

I think the auto-detection is an interesting experiment, but the underlying power to update selectively is still valuable. You won't always be able to follow the automatic inference, and you'll need the lower level API to do it anyway. So I'm most interested in getting the manual building blocks in place first, then trying to level-up on top of that afterwards.

@Daniel254
Copy link
Contributor

I just leave it here.
It's similar project with partial rendering

https://github.com/igor-alexandrov/wiselinks

@ssorallen
Copy link

This is an interesting discussion, and if you take it to the extreme of tracking every DOM node automatically you can get something like what I've put together: https://github.com/ssorallen/turbo-react

turbo-react doesn't modify Turbolinks. It monkeypatches document.documentElement.replaceChild, the method Turbolinks calls at the end of the loading process, with a version that turns the HTML into JSX, then uses the JSX parser to convert to React function calls, and then lets React do its DOM diffing and minimal updating. This limits the manual cues needed from the developer and needs no extra help from the server.

The HTML -> JSX -> React combination behaves similar to ideas mentioned above: it tracks DOM nodes with unique IDs (data-react-id) and updates only parts of the page that have changed. Node IDs are determined by their path in the tree, which means if their path changes between renders it will not be considered the same node. That's where something like data-turbolinks-partial has an advantage: the developer can mark nodes as being the same across renders even if the nodes move within the document.

@dhh
Copy link
Contributor Author

dhh commented Feb 20, 2015

Ross, that’s an interesting project. I’m curious as to when it matters to truly flow through react vs just doing these innerHTML replacements. I get it in the actual React case, because the logic is all client-side, so you’re basically just doing a redraw. But in our case, redraw only happens in response to a HTTP request. And in that case, I don’t think innerHTML is the bottleneck. The bottleneck is likely to be the remote operation, and the transfer of the response.

Happy to be enlightened to otherwise.

I like React because it’s the same motivation as Turbolinks: Avoid client-side state as much as possible! It’s just whether the updates are triggered from client or server.

On Feb 20, 2015, at 07:46, Ross Allen notifications@github.com wrote:

This is an interesting discussion, and if you take it to the extreme of tracking every DOM node automatically you can get something like what I've put together: https://github.com/ssorallen/turbo-react

turbo-react doesn't modify Turbolinks. It monkeypatches document.documentElement.replaceChild, the method Turbolinks calls at the end of the loading process, with a version that turns the HTML into JSX, then uses the JSX parser to convert to React function calls, and then lets React do its DOM diffing and minimal updating. This limits the manual cues needed from the developer and needs no extra help from the server.

The HTML -> JSX -> React combination behaves similar to ideas mentioned above: it tracks DOM nodes with unique IDs (data-react-id) and updates only parts of the page that have changed. Node IDs are determined by their path in the tree, which means if their path changes between renders it will not be considered the same node. That's where something like data-turbolinks-partial has an advantage: the developer can mark nodes as being the same across renders.


Reply to this email directly or view it on GitHub.

@ssorallen
Copy link

@dhh Yup, I don't think innerHTML is a bottleneck, and I didn't mean to imply turbo-react was fixing a bottleneck or any problems in Turbolinks. I added React as an experiment to have finger-grained automatic mutations than documentElement.replaceChild to enable things like CSS transitions. The example in the turbo-react README demonstrates a simple case:

css:

.foo { transition: background-color 0.5s; }
.bar { background-color: yellow; }
.baz { background-color: blue; }

doc1:

<body>
  <div class="foo bar"></div>
</body>

doc2:

<body>
  <div class="foo baz"></div>
</body>

Turbolinks replaces the body with the body tag of doc2, but turbo-react finds that the minimum change to get from doc1 to doc2 is actually div.className = 'foo baz' (or maybe some use of classList if supported). With turbo-react you'll see the background-color transition.

@dhh
Copy link
Contributor Author

dhh commented Feb 20, 2015

Ross, gotcha. That seems perfectly legit for that use case. Thanks for putting it out there.

On Feb 20, 2015, at 10:07, Ross Allen notifications@github.com wrote:

@dhh Yup, I don't think innerHTML is a bottleneck, and I didn't mean to imply turbo-react was fixing a bottleneck or any problems in Turbolinks. I added React as an experiment to have finger-grained automatic mutations than documentElement.replaceChild to enable things like CSS transitions. The example in the turbo-react README demonstrates a simple case:

css:

.foo { transition: background-color 0.5s
; }

.bar { background-color: yellow
; }

.baz { background-color: blue; }
doc1:

<
div class="foo bar"></div

</
body>
doc2:

<
div class="foo baz"></div

</
body>
Turbolinks replaces the body with the body tag of doc2, but turbo-react finds that the minimum change to get from doc1 to doc2 is actually div.className = 'foo baz' (or maybe some use of classList if supported). With turbo-react you'll see the background-color transition.


Reply to this email directly or view it on GitHub.

@dhh dhh closed this as completed in #468 Feb 27, 2015
@qq99
Copy link

qq99 commented Mar 16, 2015

re: @ssorallen

I added React as an experiment to have finger-grained automatic mutations than documentElement.replaceChild to enable things like CSS transitions.

That's amazing! I was just musing the other day that some kind of DOM diffing instead of outright replacements would be the only way for TurboGraft to work well with replacements & CSS transitions without adding some kind of animation lock (cc @tmlayton)

@tmlayton
Copy link

re: turbo-react; That is great. @ssorallen any reservations about moving it beyond an experiment?

@ssorallen
Copy link

@tmlayton I don't want to hijack this thread. Do you want to open an issue on turbo-react to chat? Happy to chat there.

@tmlayton tmlayton mentioned this issue Mar 16, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants