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

Proposal for new Context pattern #74

Closed
aaronmallen opened this issue Oct 16, 2019 · 6 comments · Fixed by #93
Closed

Proposal for new Context pattern #74

aaronmallen opened this issue Oct 16, 2019 · 6 comments · Fixed by #93
Labels
discussion A discussion

Comments

@aaronmallen
Copy link
Owner

aaronmallen commented Oct 16, 2019

Problem

Currently when a class inherits from ActiveInteractor::Base a context class is created and validation and attribute methods are delegated from the interactor to the context class. I wonder if this method of delegation is too "magic" or too confusing.

Proposal

I propose a new pattern for handling context. I would like to expect a context class be created in addition to the interactor class with a given naming convention. For example given an interactor named MyInteractor the gem could expect a second class MyInteractorContext to exist and if it cannot find that class it would fallback to a generic context class with no validation or attribute methods. The expectation would be instead of going the route of delegation through the interactor with methods like context_validates all validation and attribute methods would be called on the context class directly. The generators in the gem could make this easy when developing with rails.

so instead of the current pattern:

class MyInteractor < ActiveInteractor::Base
  context_attributes :first_name, :last_name, :email
  context_validates :email, presence: true

  def perform
    ...
  end
end

context = MyInteractor.perform(email: 'foo@foo.com', first_name: 'foo', last_name: 'bar')
#=> <# MyInteractor::Context ...>

the pattern could be:

class MyInteractorContext < ActiveInteractor::Context::Base
  attributes :first_name, :last_name, :email
  validates :email, presence: true
end

class MyInteractor < ActiveInteractor::Base
  def perform
    ...
  end
end

context = MyInteractor.perform(email: 'foo@foo.com', first_name: 'foo', last_name: 'bar')
#=> <# MyInteractorContext ...>

The goal here is to provide clarity on where these validations occur.

Things to consider before answering

  • What are your thoughts on this proposed pattern?
  • What are your thoughts on appropriate naming conventions (currently MyInteractor::Context is what is created)?
  • Would having the context class as an actual class make the validation and attribute methods less confusing?
@aaronmallen aaronmallen added the question A question label Oct 16, 2019
@aaronmallen aaronmallen added this to the v0.2.0 milestone Oct 16, 2019
@aaronmallen aaronmallen pinned this issue Oct 16, 2019
@aaronmallen aaronmallen added discussion A discussion and removed question A question labels Oct 16, 2019
@aaronmallen aaronmallen removed this from the v0.2.0 milestone Oct 16, 2019
@tgittos
Copy link

tgittos commented Oct 16, 2019

I definitely think the new proposed architecture is nicer.

Providing a way for the user to specify their own context class is going to make your library more extensible and powerful, and saves me as an end user from monkey patching your code anyway.

I also think context_attributes is too awkward, and much prefer your newer syntax and making it explicit that you're declaring props on a context class that you're also declaring vs an automagical declared context.

I do prefer the sub-classed Context naming convention you had before, for example:
MyInteractor::Context vs MyInteractorContext, but it's a minor thing.

@aaronmallen
Copy link
Owner Author

aaronmallen commented Oct 16, 2019

I think it would also be beneficial to allow for an interactor to define it's own context class to allow for classes that don't meet the expected naming convention.

class User < ActiveRecord::Base
end

class AuthenticateUser < ActiveInteractor::Base
  self.context_class = User
end

@tgittos
Copy link

tgittos commented Oct 16, 2019 via email

@zacharyw
Copy link

What if the default behavior is to create a class (current behavior), unless one with the appropriate naming convention already exists?

In that case the naming convention might even be overkill, and using the self.context_class is the defacto way to do it.

@aaronmallen
Copy link
Owner Author

@zacharyw so instead of falling back to a generic context class with no validations and no attribute methods you're suggesting the fallback would be the current behavior?

@zacharyw
Copy link

@aaronmallen I think so. I do see the benefit of being able to set a custom context, but for a lot of simple base case scenarios it may be annoying to have to manually keep creating new classes.

@aaronmallen aaronmallen unpinned this issue Oct 29, 2019
@aaronmallen aaronmallen pinned this issue Oct 30, 2019
@aaronmallen aaronmallen added the help wanted Extra attention is needed label Oct 31, 2019
Repository owner locked and limited conversation to collaborators Dec 10, 2019
@aaronmallen aaronmallen unpinned this issue Dec 10, 2019
@aaronmallen aaronmallen removed the help wanted Extra attention is needed label Dec 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
discussion A discussion
Projects
None yet
3 participants