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

Support Polymorphic Associated Objects #25

Closed
wants to merge 2 commits into from

Conversation

kaspth
Copy link
Owner

@kaspth kaspth commented Jun 2, 2024

Sometimes there's certain behavior you'd like to share between multiple objects. However, Associated Object's design was purposefully written to only support associating with one distinct class.

This tries to shimmy that open a bit more, so you can do:

class Pricing < ActiveRecord::AssociatedObject::Polymorphic
end

class Post::Pricing < Pricing
end

class Comment::Pricing < Pricing
end

cc @garrettdimon @julianrubisch @natematykiewicz

Sometimes there's certain behavior you'd like to share between multiple objects.
However, Associated Object's design was purposefully written to only support associating with
one distinct class.

This tries to shimmy that open a bit more, so you can do:

```ruby
class Pricing < ActiveRecord::AssociatedObject::Polymorphic
end

class Post::Pricing < Pricing
end

class Comment::Pricing < Pricing
end
```
@@ -1,4 +1,13 @@
class ActiveRecord::AssociatedObject
class Polymorphic < self
def self.inherited(klass)
if name = klass.module_parent_name
Copy link
Owner Author

Choose a reason for hiding this comment

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

I'm using/abusing the module_parent_name to detect Post::Pricing versus just Pricing and then only firing the connecting behavior when we're doing Post::Pricing. So technically we don't need a separate Polymorphic object to capture this and we could stash this condition in ActiveRecord::AssociatedObject.

Yeah, having written it out now, I actually think that's more intuitive and it simplifies our object hierarchy.

# Here, `record` will return the post for Post::Pricing objects and the comment for Post::Comment::Pricings.

def something_conditional
execute_behavior if post?
Copy link
Owner Author

Choose a reason for hiding this comment

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

I'm trying to figure if it makes sense to add these post? and comment? shorthand methods to allow conditional behavior in the polymorphic parent class. Or if it's better to just rely on OOP and define a basic implementation in the polymorphic parent class that the subclasses can still enrich.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's always tough to know without using it in production code, but my gut says this would lead to wonky things. I like the idea of keeping it 100% shareable so the associated object can focus only on the things that are universal to all of the classes that will use it.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, that does make sense to me and I think you're right about skipping that support for post?/comment?.

end
end

class Post::Pricing < Pricing
Copy link
Owner Author

Choose a reason for hiding this comment

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

I was wondering if I should omit having the namespaced Post::Pricing classes and try to just make Pricing work everywhere. But I think it breaks the whole conceptual model of Associated Objects.

One other thing, I've been considering is to somehow support this:

# app/models/pricing.rb
class Pricing < ActiveRecord::AssociatedObject
  class Post::Pricing < self # Post could be loaded here!
  end

  class Comment::Pricing < self
  end
end

However, Post.has_object :pricing would fail because there's no post/pricing.rb file. We could instead try "Pricing".constantize which would work, and then skip our Associated Object eager-loading attempt …because it'd be defined right after Post finishes loading. I hope this makes sense! All this kind tricky autoloading behavior makes me feel a bit uneasy.

Copy link
Contributor

Choose a reason for hiding this comment

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

This was the first thing that popped into my head. If associated objects are shared, where do they live? It feels like the non-polymorphic approach makes it very clear cut that it's a way of organizing code rather than sharing code. (Whereas concerns, for example, are primarily about sharing code.)

@kaspth
Copy link
Owner Author

kaspth commented Jun 2, 2024

An out-of-the-box alternative could be recommending using a concern like this:

class Post::Publisher < ActiveRecord::AssociatedObject
  include ::Publisher
end

class Comment::Publisher < ActiveRecord::AssociatedObject
  include ::Publisher
end

# app/models/concerns/publisher.rb
module Publisher
  extend ActiveSupport::Concern
  
  included do
    extension do
      # Feels a bit clunky though.
    end
  end
  
  # Any regular instance methods too
end

Not sure how I feel about the extension block nested in the included block, but it would work.

@garrettdimon
Copy link
Contributor

An out-of-the-box alternative could be recommending using a concern like this...

I agree about the nested extension block, but honestly, I kind of like how this uses AssociatedObject as organization but then uses Concerns for sharing. Again, without using it day-to-day in some production code, I'm not sure how strong my opinion is. I just think there's something beneficial about "Concepts" (AssociatedObject, Concern) having one specific task that they excel at (AO: Organizing, Concern: Sharing) so it's clear when to use which one.

@kaspth
Copy link
Owner Author

kaspth commented Jun 2, 2024

@garrettdimon yeah, I like the way you put this. I also wonder how often people would need to inject an extension block in the shared concern, since it hooks into the class it might be very class specific — and then would be better placed in individual extension blocks in Post::Pricing and Comment::Pricing.

@natematykiewicz
Copy link
Contributor

When we use extension it's always to set up callbacks to run on the model that use the associated object.

@kaspth
Copy link
Owner Author

kaspth commented Oct 3, 2024

We ended up deciding this wasn't worth it and code sharing may be better handled in concerns for now.

@kaspth kaspth closed this Oct 3, 2024
@kaspth kaspth deleted the polymorphic-associated-objects branch October 3, 2024 09:05
kaspth added a commit that referenced this pull request Dec 10, 2024
Feels a bit iffy, but it's easier for us to support versus trying to bake in "polymorphic" support via inheritance.

See #25 and #29 for the previous discussion.
kaspth added a commit that referenced this pull request Dec 10, 2024
…-modules

Feels a bit iffy, but it's easier for us to support versus trying to bake in "polymorphic" support via inheritance.

See #25 and #29 for the previous discussion.

So given the tradeoffs I think this is the best we've got.
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