Merged
Conversation
leastbad
reviewed
Jan 5, 2022
Contributor
leastbad
left a comment
There was a problem hiding this comment.
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. 😸
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds a
skip_updatesoption 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.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_updatesto 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.