Callbacks proxy

[julianrubisch]

Third iteration:

This time, the API is confined to the ActiveRecord attributes class methods.

When calling a Kredis type in a model class like so:

  kredis_list :names_with_after_change_proc_callback, after_change: ->(p) {}
  kredis_list :names_with_after_change_method_callback, after_change: :changed

  def changed(p)
  end

proc or method type callbacks are executed, and the method instance passed to them.

One variant would be to also pass the Kredis attribute (the set, list, etc.) to the callback as a second argument to save some characters when accessing it in the callback.

@dhh I would appreciate quick nod if this is what you had in mind, thanks! I will then wrap it up with more tests and docs.

[julianrubisch]

Actually, forgot to define which methods to wrap, else we're ending up in an infinite loop... nothing to see here 🙈

[julianrubisch]

Yep, now it's looking good 🙂

[dhh]

Yeah, this is exactly the path I was thinking 👍

[julianrubisch]

Great! I'll flesh this out more in the coming days.

[julianrubisch]

@dhh most of the functionality should now be "done", I'll add more tests and documentation, though there's one remaining question: what's the preferred signature of the callbacks?

Right now it's being called with the record, but my gut feeling says that it should (also) be passed the kredis_type, because typically I'd probably want to access the mutated set/list/..., and not the record?

[leastbad]

@dhh I've been doing my best to follow along with the saga of this feature, and I'm super excited to start using this implementation.

However, it is currently constrained to the AR model methods. Going back to re-read what you said here, I suspect that my initial interpretation of your words was incorrect. I thought you were talking about the Redis connection instance, not an AR model instance. So suddenly I realize that you appear to be saying that "after callbacks on a simple type are only useful for jobs" and that isn't a strong use case. Is that accurate?

If so, I want to urgently implore you to reconsider: calling a job after a simple value type is updated is my number one hope and ambition for this feature! I am a heavy user of simple value types and the use cases for callbacks on Kredis.string alone are a page long.

Now that we're thinking in terms of reactive UIs, being able to make things happen when data is updated isn't an edge case... it's everything.

I checked in with Julian to discuss and he's really ready for this feature to be done. After several re-writes, I don't blame him. I am happy to pick up the baton and run with it - whether that means attempting to contribute to this PR or opening a next one to handle simple types - but clarification from you before I invest would really be appreciated.

[julianrubisch]

I think this is pretty much done TBH. I will add more tests and docs today.

Note that if I'm not mistaken you can use the proxy with a "normal" kredis type if you just omit the record and ensure to supply a callable (Proc)

Maybe one could add a convenience factory function?

Kredis.with_callback :set, after_change: ->(set) {}

[julianrubisch]

Done!

Note that I've changed the signature of the callback to record, type, so that you get both the resource as well as just the kredis type the mutations happened on

[leastbad]

Just to follow-up on my last comment, I believe that what I'm looking for is not a callback but a kind of observer - and it's way, way out of scope for Kredis.

While the spirit of my desire is intact, I don't think what I was describing belongs in this library and I hope Julian's work can be merged without me muddying the water further.

[dhh]

We're getting close. This is great!

[dhh]

Think assigning the proxy to an explaining variable could help with the multi-line nature here.

[dhh]

Don't think the instance method callback should probably take a parameter, yeah? Like they don't in Active Record/Model. Only the procs do that.

[dhh]

This feels like a configuration-level method that shouldn't be exposed directly through the API. What about using a constant instead?

[dhh]

Then you also don't have a to call freeze. And for style, space after opening and before closing 👍

[julianrubisch]

Makes sense!

[julianrubisch]

I'm too used to standard 😅

[julianrubisch]

Done! (I think)

[kaspth]

Hey, sorry for the long wait on this from me and thanks for all the work on this. My thinking on the implementation here was different enough that I've pushed an alternate take: https://github.com/rails/kredis/compare/after-change-invocations

I find the callbacks proxy somewhat overwrought for this, and I'd rather we also support the Kredis.set(…, after_change: ->(set) { set.members }) case since we're going to the trouble of adding this. For new types or adding new operations to existing types, I think we're better off having the after_change markings in the types themselves — Kredis won't be seeing lots of contributing action, so this type of stuff is easy to forget after months of inactivity.

I'd also like to add some automated checks, so we can catch the case where a new operation was added but the after_change marking was forgotten. As well as the type specific callback tests.

From looking into this, I also found out that both these implementations are incompatible with multi/pipelined iirc if there's any reads in the block. To support that we should defer any after_change callbacks to run after the multi block ends, or note the gotcha for now.

[julianrubisch]

Hm that's somewhat similar to one of my earlier takes 🤔

[leastbad]

@kaspth, it seems like you've reimplemented #34, which Julian wrote after @dhh gave feedback on #33.

Between you and David, you have Julian on a hamster wheel. We all want an ideal outcome... but come on.

[dhh]

You could certainly go in both directions on this, and the end-user API remains the same, but I like where @julianrubisch has ended up treating the after_change callback structure as essentially an aspect. Cross-cutting feature that the individual types don't need to be concerned about. So given the work @julianrubisch has put into this over several weeks, and the fact that the implementations achieve the same goal, I'd rather we stick with that, as long as the concern around multi can be addressed.

[julianrubisch]

as long as the concern around multi can be addressed.

So what's the contingency plan for this? Document it? Wrap it in a mutex? Not sure how to address it...

And re cross cutting concerns, as noted above one could probably conceive convenience functions to address this, e.g.

Kredis.with_callback :set, after_change: ->(set) {}

Not 100% sure how the implementation would look but I think it could be done. (The proxy essentially doesn't care where it sends actions to)

[dhh]

I think we're fine to note the gotcha / investigate a solution in another PR for now 👍

[dhh]

Think we have an issue with the CI. Tests are passing locally. So merging.