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

fix updates_for makes redundant calls #225

Merged
merged 1 commit into from
Dec 24, 2022
Merged

Conversation

leastbad
Copy link
Contributor

Type of PR (feature, enhancement, bug fix, etc.)

Bug fix

Description

Modify updates_for element to verify that a page is required before fetching it.

This PR essentially implements the suggestion made in #209, with minor tweaks.

I've done my best to test this new functionality, but I am not a heavy user of updates_for and would appreciate if @julianrubisch could sanity check this. Specifically, my concern is that there could be an unexpected interaction with lazily loaded Turbo Frame content?

I was concerned that filtering out blocks might cause the index-based element selection to point to the wrong element, but so far, I haven't been able to trip it up. I really don't want to break this functionality.

@andrewerlanger I also removed the previous line 117, which was where we performed the shouldUpdate check initially. It seems as though this should no longer be necessary. Is there a good reason to run the check again?

I've tested on a simple User model with enable_updates, as well as name and email attributes:

<%= updates_for current_user, only: :email do %>
  <p><%= current_user.name %></p>
  <p><%= current_user.email %></p>
<% end %>

<%= updates_for current_user, only: :name do %>
  <p><%= current_user.name %></p>
  <p><%= current_user.email %></p>
<% end %>

Seems to work fine when I update either attribute.

Fixes #209

Why should this be added

@andrewerlanger pointed out that we fetch pages from the server before checking if they are used, which is a waste of computing resources and bandwidth.

Checklist

  • My code follows the style guidelines of this project
  • Checks (StandardRB & Prettier-Standard) are passing
  • This is not a documentation update

@leastbad leastbad added bug proposal javascript Pull requests that update Javascript code labels Nov 19, 2022
@leastbad leastbad added this to the 5.0 milestone Nov 19, 2022
@leastbad leastbad self-assigned this Nov 19, 2022
Copy link
Contributor

@julianrubisch julianrubisch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a sane approach 👍🏻

I'll see if I can check this against an app of mine.

Would be nice if @andrewerlanger did the same, provided you still use it.

@leastbad
Copy link
Contributor Author

@julianrubisch @andrewerlanger any feedback so far? I'd love to get this merged if everything looks okay.

@leastbad leastbad merged commit f4b08e5 into master Dec 24, 2022
@leastbad leastbad deleted the updates_for_redundant_calls branch December 24, 2022 20:04
@andrewerlanger
Copy link
Contributor

@julianrubisch @leastbad not sure how I missed these messages but only just seeing them now. Sorry for the radio silence and many thanks for pushing ahead with the fix :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug javascript Pull requests that update Javascript code proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

updates_for Makes Redundant Calls
3 participants