-
-
Notifications
You must be signed in to change notification settings - Fork 72
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
Debounced updatable #261
Debounced updatable #261
Conversation
✅ Deploy Preview for cableready ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
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.
This is looking great.
@@ -30,11 +50,14 @@ def self.enable_cable_ready_updates(*options) | |||
options = options.extract_options! | |||
options = { | |||
on: [:create, :update, :destroy], | |||
if: -> { true } | |||
if: -> { true }, | |||
debounce: 0.seconds |
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 wonder if it would make sense to have the default provide some degree of protection. Perhaps 100 milliseconds?
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.
well I switch on the debounce time in broadcast_updates
to gracefully allow the default behavior. But sure, why not!
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.
actually I just noticed that this breaks all the tests, so we'd have to specify debounce: 0
everywhere. I'd recommend merging this PR and then introducing this in a separate one to keep things readable
Also note that this doesn't yet factor in the case of |
Will association support land in a separate PR? |
I thought it would be wiser to split it, yes |
1f32d13
to
0288894
Compare
Enhancement
Description
One of the big downsides of using
CableReady::Updatable
in the wild is that it can lead to a lot of noise on the ActionCable connection. This is because every change on a model that enables updates leads to a ping being sent over ActionCable due to the usage of ActiveRecord callbacks.This PR introduces a
debounce:
option that takes anActiveSupport::Duration
argument that will only send out one ping for this model per debounce period.It makes use of a simple internal per-process
MemoryDebounceAdapter
, which is just a thin synchronized layer around aHash
.Preparations are made, however, to override this adapter using a
CableReady::Updatable.debounce_adapter
mattr_accessor, which can be overridden in an initializer. This should make it possible to devise other adapters (for example based on DRb or Layered Caching) in the future.Checklist