Include changeset in class-level updates broadcast

[grncdr]

Enables using the :only option with updates_for to filter updates when subscribing to all instances of a model class.

Fixes #192

[julianrubisch]

Okay I expected that test failure, that's okay.

There also don't seem to be any tests for this behavior yet which is an omission on our part. I'll write some tests for this soonish.

The feature itself looks sensible, though I'd also like to ask @andrewerlanger for his opinion since he originally proposed this feature and maybe has a fresher memory of things than me.

[grncdr]

@julianrubisch I'm not sure what you mean by "There also don't seem to be any tests for this behavior yet". I looked at the test failures, and they were mostly/all tests for this behavior, so I've updated the expectations.

[julianrubisch]

Sorry for being unclear - I meant there's currently no tests for a specific case testing the only option. Not related to this PR at all.

I'd still love for @andrewerlanger to check back on this in a spare minute to make sure I don't miss something!

[andrewerlanger]

@grncdr thanks very much for taking this on 🙏

The code itself looks good to me, I'm just not sure whether the following is true:

when subscribing to all instances of a model class.

As far as I understand: when passing a class constant to updates_for, you don't fully subscribe to all instances of that class, but rather to create and destroy callbacks specifically. This is what makes it a good candidate for index views.

If that is indeed the case, it would mean that the changes introduced in this PR do the following for the code below:

<%= updates_for Item, only: :title do %>
  <% items.each do |item| %>
    <p><%= item.title %></p>
  <% end %>
<% end %>

• If an item is created with a title: it gets appended to the list
• If an item is created without a title: the list remains unchanged
• If you update an item's title: the list remains unchanged

From the PR description, it sounds to me like that last point is the important one. If you update an item's title, the list should be changed (specifically, the new title should be displayed). But I don't think that will be the case here.

I could very well be wrong about this though, so all the more reason to add some tests :)

[grncdr]

Thanks for checking this out @andrewerlanger 👍

From the PR description, it sounds to me like that last point is the important one. If you update an item's title, the list should be changed (specifically, the new title should be displayed). But I don't think that will be the case here.

There's some confusion here about what problem this PR is actually solving, probably because I didn't include the issue description from #192 here. My issue is not that updates were being missed, but that updates_for was causing (many) reloads for updates that were uninteresting. Sticking with the item.title example, imagine if Item also contained a view_counter that was being updated every few seconds. Given your example code, everybody viewing items is going to reload the list every few seconds, even though the item titles have never changed.

Edit: given this explanation, I can attest that these changes do work as described, because I've been using a monkey-patch version of this in production since I opened the original issue.

[grncdr]

What follows is a lot of pedantic clarification, feel free to skip it if we're already on the same page.

As far as I understand: when passing a class constant to updates_for, you don't fully subscribe to all instances of that class, but rather to create and destroy callbacks specifically.

That is not the case. If the model class contains enable_updates and the :on option includes :update (which it will by default), it will broadcast to the model.class channel after any update on the first line of broadcast_update:

cable_ready/app/models/concerns/cable_ready/updatable/model_updatable_callbacks.rb

Lines 22 to 25 in 50ffe49

	def broadcast_update(model)
	model.class.send(:broadcast_updates, model.class, {})
	model.class.send(:broadcast_updates, model.to_global_id, model.respond_to?(:previous_changes) ? {changed: model.previous_changes.keys} : {})
	end

(this commit is the main branch at the time I made the comment).

The model.class channel is of course the same channel that updates_for Item do ... end is subscribed to, but because the broadcast data does not contain the "changed" key, the only: option is ignored:

cable_ready/javascript/elements/updates_for_element.js

Lines 190 to 199 in 50ffe49

	hasChangesSelectedForUpdate (data) {
	// if there's an only attribute, only update if at least one of the attributes changed is in the allow list
	const only = this.element.getAttribute('only')
	return !(
	only &&
	data.changed &&
	!only.split(' ').some(attribute => data.changed.includes(attribute))
	)
	}

Regarding the scenarios you described:

• If an item is created with a title: it gets appended to the list
• If an item is created without a title: the list remains unchanged

The broadcasting of updates for items being created (or destroyed) goes through a different code path, calling broadcast_create unless the user has specifically opted out with the on: option to enable_updates.

• If you update an item's title: the list remains unchanged

This is also incorrect, the list will be refreshed, both in the current version and in the version including my changes. However, if some other attribute of the Item is updated, the list will be refreshed in the current version, and won't be refreshed after my PR.

All this PR does is add a little more information to that existing broadcast so that the updates_for component can ignore updates it doesn't care about.

[andrewerlanger]

@grncdr thanks for clarifying, it sounds like I was wrong in that case. I thought something like <% updates_for Item %> would only hook into the create and destroy callbacks, which would have meant an update to e.g. item.view_counter wouldn't interfere with the list of items.

If, as you suggest, all items are indeed being updated for uninteresting updates, then I fully agree it would be useful to opt out of these. That was also my motivation for proposing an only: option at the level of individual model instances in the first place.

Appreciate you taking the time to explain this in detail. Everything looks good to me with the help of that context (although with the minor caveat that I haven't had a chance to test it myself yet).

@julianrubisch @leastbad what do you think?

[julianrubisch]

Thanks @grncdr for the detailed rundown. 💚

I admit that even though writing most of it I'm sometimes fuzzy wrt the code paths at play.

That part is also hard to test because it involves an interaction of backend and frontend code.

I will reserve some time soon to write a couple of JavaScript tests that should hopefully clear things up for the next time I need to look at it 😬