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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add skip_updates option #168

Merged
merged 1 commit into from
Jan 5, 2022
Merged

Conversation

assuntaw
Copy link
Contributor

@assuntaw assuntaw commented Dec 2, 2021

This adds a skip_updates option to wrap any database CRUD changes in that should not be broadcast, for example because they are happening in an environment without a running Redis server.

# app/models/application_record.rb
class ApplicationRecord < ActiveRecord::Base
  include CableReady::Updatable

  self.abstract_class = true
end

# db/seeds.rb

# Skips broadcasts for all models inheriting from ApplicationRecord:
ApplicationRecord.skip_updates do
  User.destroy_all
  User.create!(user_attrs)
end

# Skips broadcasts for User only:
User.skip_updates do
  # will not broadcast
  User.destroy_all
  User.create!(user_attrs)

  # will broadcast
  Post.destroy_all
  Post.create!(post_attrs)
end

Big thanks to @leastbad and @julianrubisch for pointing me in the right direction 馃 I decided to borrow heavily from ActiveRecord's no_touching. Limiting skip_updates to the current thread (rather than working with a class variable) means that we don't skip broadcasts for other models or, crucially, other clients. I think that this is the safest option, and can't really think of a use case for one client prohibiting broadcasts for all other clients.

@assuntaw assuntaw marked this pull request as ready for review December 6, 2021 15:39
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.

LGTM!

Copy link
Contributor

@leastbad leastbad left a comment

Choose a reason for hiding this comment

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

I apologize for the delay in getting this approved. I am very impressed with the simplicity of this solution. I would like to understand how it works a bit better so that I'll be able to support and potentially extend it in the future. However, this review is not intended as a blocker and I would be comfortable merging it right away if @julianrubisch is comfortable with everything. My understanding can come as a step 2 in this case. 馃樃

app/models/concerns/cable_ready/updatable.rb Show resolved Hide resolved
app/models/concerns/cable_ready/updatable.rb Show resolved Hide resolved
@leastbad leastbad merged commit ca07c8e into stimulusreflex:master Jan 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants