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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow Associated Objects to extend their Active Record #19

Merged
merged 7 commits into from
Jan 7, 2024

Conversation

kaspth
Copy link
Owner

@kaspth kaspth commented Dec 23, 2023

If we eager load the Associated Object class when calling has_object we can have an extension class method fire that can extend the Active Record class.

class Post::Publisher < ActiveRecord::AssociatedObject
  extension do
    has_many :contracts, dependent: :destroy

    after_create_commit :publish_later_after_signed

    private def publish_later_after_signed
      # An integrating method that operates on `publisher`.
      # A little tough to generate an example since we're now mixing in a `contracts` concept.
      publisher.publish_later if contracts.all?(&:signed?)
    end
  end
end

I'm still not quite sure how I feel about this. I'll write some documentation later, after I've sat with this some more.

@garrettdimon this doesn't quite get us to the automatically scoped validations, but is this closer to what you were talking about with replacing those wrapping ActiveSupport::Concerns ala kaspth/conventional_extensions?

@natematykiewicz is this something that would be useful to you? Does this seem clear enough? Would it break your app if we eager-loaded the Associated Objects (due to autoloading it should be fine)?

@kaspth
Copy link
Owner Author

kaspth commented Dec 23, 2023

@seanpdoyle have you tried using this gem yet? Would this be something you'd be interested in?

@natematykiewicz
Copy link
Contributor

I'm not sure if I'd use that feature or not, but it makes sense. Also, I think this feature or not, we should eager load (when eager loading is enabled) AssociatedObject and ActiveJob Performs classes. If they aren't eager loaded in production, we'd end up using more memory than necessary.

@natematykiewicz
Copy link
Contributor

Also, I don't think this would cause me any problems. I've assumed they work like models where they're eager loaded in production and lazy loaded in development. If not, I think we should change that.

@garrettdimon
Copy link
Contributor

I think I'd have to tinker with it and see how it feels, but just looking at it, it sure seems like a good approach.

@kaspth
Copy link
Owner Author

kaspth commented Dec 28, 2023

@garrettdimon nice! There's also no rush on this, so take your time. I wonder how we explain it to people, since it could be a bit hidden. I could see an alias_method :has_extending_object, :has_object to help clarify that something happens in the other file. Of course, post.method(:<something>).source_location still points to the right location.

@kaspth
Copy link
Owner Author

kaspth commented Dec 28, 2023

I'm not sure if I'd use that feature or not, but it makes sense. Also, I think this feature or not, we should eager load (when eager loading is enabled) AssociatedObject and ActiveJob Performs classes. If they aren't eager loaded in production, we'd end up using more memory than necessary.

@natematykiewicz we're just leaning on app/models being set up for eager-loading so both Associated Objects and ActiveJob::Performs will be eager-loaded when the app is.

Also, I don't think this would cause me any problems. I've assumed they work like models where they're eager loaded in production and lazy loaded in development. If not, I think we should change that.

Yeah, that's how it works before this PR. However, if we shift to loading the Associated Object when has_object is called, then we can inject extensions into the record class — and obviate some needs for wrapping ActiveSupport::Concerns.

Basically, using the example from the description, before this PR it'd be common to do something like this:

# app/models/post.rb
class Post < ApplicationRecord
  include Published
end

# app/models/post/published.rb
module Post::Published
  extend ActiveSupport::Concern

  included do
    has_many :contracts, dependent: :destroy

    has_object :publisher
    after_create_commit :publish_later_after_signed
  end

  private def publish_later_after_signed
    publisher.publish_later if contracts.all?(&:signed?)
  end
end

So we're splitting the extension/integration into a wrapping Concern, but what if we could bundle that into the Associated Object now that they're a domain concept and we're already using them? It gets tricky though, especially communicating where things come from.

@natematykiewicz
Copy link
Contributor

I realized yesterday that I'd probably move the callbacks inside the extension block. I use associated objects with activerecord callbacks a lot. Oftentimes its after_create_commit and then an after_update_commit if saved_change_to some field(s). The after_create_commit doesn't have conditions, the code often looks like:

# model
has_object :foo, after_create_commit: :sync_later
after_update_commit -> { foo.sync_later }, if: :saved_change_to_active?

But with extensions I'd probably just switch that to:

# model
has_object :foo

# associated object
extension do
  after_create_commit -> { foo.sync_later }
  after_update_commit -> { foo.sync_later }, if: :saved_change_to_active?
end

Ship it, Kasper!

@kaspth
Copy link
Owner Author

kaspth commented Jan 7, 2024

@natematykiewicz nice! Sounds good, I think I'll ship this after we get #20 in then, and cut a new release.

@kaspth kaspth force-pushed the extension-on-object branch from b17a486 to 853768a Compare January 7, 2024 21:11
kaspth added 2 commits January 7, 2024 22:13
Kredis now expects its connector to respond to `root`,
which is a subtle breaking change when its assigned to something other than the default `Rails.application`.
@kaspth kaspth merged commit 8568524 into main Jan 7, 2024
2 checks passed
@kaspth kaspth deleted the extension-on-object branch January 7, 2024 21:29
@natematykiewicz
Copy link
Contributor

@kaspth in your rebase, you lost the eager loading of the associated object class.

natematykiewicz added a commit to natematykiewicz/active_record-associated_object that referenced this pull request Jan 22, 2024
PR kaspth#19 previously loaded the associated object class when the has_object
was defined, so that extensions would get run. It seems like that code
got lost in a rebase. So now, extensions only run on boot if eager
loading is enabled (which it is in production). Otherwise they aren't
loaded until the first time the associated object gets used.
kaspth pushed a commit that referenced this pull request Jan 24, 2024
PR #19 previously loaded the associated object class when the has_object
was defined, so that extensions would get run. It seems like that code
got lost in a rebase. So now, extensions only run on boot if eager
loading is enabled (which it is in production). Otherwise they aren't
loaded until the first time the associated object gets used.
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.

3 participants