Pattern for Breaking Circular Dependencies in Rails Models - Feedback and Suggestions Welcome!

thanks for your reply, Philip. I’ll see what I do.

Message originally sent by slack user U70TIGAX94P

If the extension (adding the callback) is only executed when the class is loaded, which is what load hooks will do for you, there should be no problem.

To_prepare will then only re-register the load hook, which should be fine

One possibility would be to add this at the end of my ApplicationRecord class

ActiveSupport.run_load_hooks("#{self.name.underscore.gsub("/", "__").to_sym}_model_loaded", self)

Then I could do on_load(:post_model_loaded) or on_load(:something__nested_model_loaded) any other model. But it might be too broad. :confused:

BTW… it looks like Zeitwerk’s on_load would allow me to monitor the loading of a specific class. So something like this could work:

Rails.autoloaders.main.on_load("Post") do |klass, abspath|
  # klass is the reloaded Post class so I can (re-)configure it here
end

Message originally sent by slack user U70TIGAX94P

Mmh, intriguing

Message originally sent by slack user U70TIGAX94P

I believe run_load_hooks will run the load hooks when it‘s called, not when the given class is loaded, so putting it on ApplicationRecord would not work as expected

It needs to be outside of the class anyway so you can’t inherit it

Message originally sent by slack user U70TIGAX94P

Zeitwerk‘s on_load looks interesting indeed. I‘ll look into it. I wonder how well it plays with rails‘ reloading

Message originally sent by slack user U70TIGAX94P

Also will it trigger on something loaded via require? :thinking_face:

Message originally sent by slack user U70TIGAX94P

Seems like it does all the right things. There is one remaining problem though. Packwerk doesn’t understand this is a constant reference. In the pattern we have in place right now we use an actual constant reference that is lazy because it is in a lambda, in addition to the load hook key.

I guess we could still have a load hook key, which would be the string name of the referenced constant, but I haven’t found how to make that look not horrible

And what if we ignore klass and just use Post inside the on_load block? Wouldn’t packwerk find that?

Message originally sent by slack user U70TIGAX94P

Yeah you could do that. Just make sure you don’t accidentally modify Post in the load hook for Comment…

Message originally sent by slack user U70TIGAX94P

One reason why we have a wrapper around it, we check that

In the pattern we have in place right now we use an actual constant reference that is lazy because it is in a lambda, in addition to the load hook key.

Do you happen to have a ready to share example of this? No presure. heh

I like the simplicity of just hooking in on the initializer (from your original gist), but would there be a benefit to decoupling from using ActiveSupport Notifications instead? Let the Post do a broadcast in its own callback, and then the commenting package just writes up a Subscriber to listen for the event, with a post_id and does its cleanup without strongly depending on the Post model by name.

Message originally sent by slack user U70TIGAX94P

Spicy take maybe but I think it’s good to depend by name for this. Otherwise the strong dependency still exists, you’ve just obfuscated it.

Is that really still a strong dependency? We’ve got some vague future architecture plans at work where we want to actually decompose our monolith into independent services, so that’s the direction I’m coming from.
Using AS Notification/Subscription, it’d be reasonably easy to route those notifications to an external message bus to propagate, I think. Higher level app would need to change to now load the comment data externally ofc, but the data sync hooks for data cleanup chains would be lower churn.

I have opinions about that plan, and if we’d ever get there, and agree the simple code probably wins out for most folks.

Message originally sent by slack user U70TIGAX94P

Subscribers always depend on the API contract defined by the publisher, to some degree.

Replying late here, but still on point for @jamie_ca: If you’re planning to actually split your packs into separate apps, then you’ll be forced to something more in line with notifications and a message bus. But the pattern I presented is for when you’re splitting in packs but not planning to break the monolith. The way I presented it, you’re still in the same DB transaction, and that better resembles the expected behavior of a dependent option in a has_many/has_one.