-
Notifications
You must be signed in to change notification settings - Fork 5
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
WIP - Interactor.Builder #15
base: master
Are you sure you want to change the base?
Conversation
Heavily influenced by the DSL and code of Plug.Builder.
cc @beerlington @totallymike a start on #13. Your thoughts on deprecation/rollout would be appreciated. |
Maybe we put this behavior in a different module for a point release or two. For simple interactors, one can temporarily
Or, depending on how quickly we want to move forward, we can put the old style into an Interactor.Old module for a minor release or two. Thankfully this isn't at 1.0 yet! |
To be honest I am leaning towards either supporting a 0.1.0 branch or Intector.{Old|Legacy|Simple} or something. There are some pretty significant differences, in particular the response will ALWAYS be an %Interaction{} and I would like to remove call_task and call_async in favor of a |
Another reason to just maintain it in a branch is the ability to drop the Ecto dependency (even if it is optional). |
6b862c1
to
223ab7a
Compare
Enables: * Supporting async/task as strategies (opt `strategy: :async`) * `Interactor.call/2` always returns an `%Interaction{}`
223ab7a
to
ac3b2a9
Compare
This also allows easy extension for other strategies such as Exq, Toniq, worker pools, gen server, etc.
@beerlington @totallymike I believe this is ready for some testing and feedback. The big question still up in the air is still how much backwards compatibility to try to maintain. The new code and API should be much more sophisticated, composeable, and flexible while serving the same purpose but it is so completely different that it could practically be another library. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks awesome. I left some questions here and there, but nothing I would consider blocking.
@doc false | ||
defmacro __before_compile__(env) do | ||
interactors = Module.get_attribute(env.module, :interactors) | ||
{interaction, body} = Interactor.Builder.compile(env, interactors) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fascinated by what's happening between Interactor.Builder.compile/2
and interactor_builder_call/2
. It looks like it's taking all the interactors declared in the builder and wrapping them up in a big old nest, while also unpacking their individual guard clauses to catch them in the success field? That's pretty cool.
For my own edification, I'll have to pull down this PR and try to expand the macros to see what it all compiles out to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is pretty much exactly how Plug.Builder does things, with a few tweaks to. I honestly haven't tried any guard stuff here, but I kept it around.
It is basically a huge nested case statement when compiled, the reduce building up the AST with each interactor added.
quote do | ||
@behaviour Interactor | ||
import Interactor.Builder, only: [interactor: 1, interactor: 2] | ||
import Interactor.Interaction # TODO, is this a good idea? assign/3 could conflict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you worried about assign/3
conflicting with user functions? That's probably a reasonable concern. At least Interactor.Interaction
is pretty small; maybe we reference the functions by their full name, or just alias Interactor.Interaction
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Come to think of it, I'm having trouble figuring out why we're importing Interactor.Interaction
here. Is it a convenience measure for folks use
ing the builder module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RE conflicts: assigns/3 is also often imported by Plug.Conn and Phoenix.Socket. I have removed the import in favor of an alias. It is just a convenience.
%Interaction{} = interaction -> | ||
Interaction.add_rollback(interaction, rollback) | ||
{:error, error} -> | ||
Interaction.rollback(%{interaction | success: false, error: error}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I dig the whole rollback sequence.
Would it make sense to also have
%Interaction{success: false} = interaction ->
Interaction.rollback(interaction)
to allow interactors to return their own Interaction
with the failure state already established?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yeah, good call. I will add that.
test "rollback/1" do | ||
assert %Interaction{} = int = Interactor.call(Two, %{zero: 0}) | ||
assert %{zero: 0, two: 2} = int.assigns | ||
int = Interaction.rollback(int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I super dig that you can explicitly roll back a finished interaction. That hadn't even occurred to me, and I like the way this is accomplished.
use Interactor.Interaction | ||
import Ecto.Changeset | ||
|
||
interactor :post_changeset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about this DSL the other day and I think interactor
here is kind of misleading. It seems like it's really a step within an interator, rather than actually calling an interactor itself. Is there a reason you went with that instead of a verb like perform
or execute
or a noun like step
or action
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think of it exactly like I think about Plug. A Plug is either a module or a function that accepts a Plug.Conn and returns a Plug.Conn. The DSL is simply listing which plugs (either module or function) to be called in what order.
An interactor is the exact same thing, either a module or a function that accepts an Interaction and returns an Interaction*. The DSL is just which interactors are to be called.
* It can return other values too, which are assigned to the Interaction.
Does thinking about it that way change how you perceive it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the analogy, but I think maybe what I'm hung up on is "what is an interactor?". Is CreatePost
the interactor or is post_changeset
? My understanding was that CreatePost
is the interactor, and this DSL is telling the interactor to perform a task. I'm basing that on https://github.com/collectiveidea/interactor#what-is-an-interactor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To continue the analogy: "what is a plug?"
The answer to me is an interactor module or function that receives an interaction and does something with it. It is simply a pattern to help build easy to follow, simple, maintainable code.
Either way I think this is a minor naming issue compared to verifying the actual functionality and figuring out he best way forward with this very breaking change.
Goal here is to remove the existing
before_call
,handle_call
,after_call
callbacks in favor of a plug like interface for building up sophisticated interactions.Simple interactors can be a simple module with a call/2 function:
Complex interactors can be a build up using Interactor.Builder:
Interactors called via a builder can return:
value
, will be added to assigns as:assign_to
opt or name of interactor.{:ok, value}
will also be assigned as First pass on hooks. #2.{:error, value}
will be added to :error key of struct and pipeline halted.An integrated Interactor.Ecto package will provide automatic insertion and assignment of %Ecto.Changeset{} and %Ecto.Multi{} data structures. (We can remove dependency with removal of old behaviour though).
Rolling back can b
TODO:
assign_to
opts.async
opts.rollback
opt/support.