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

Add before and after hooks to store middleware #173

Merged
merged 7 commits into from Dec 14, 2018

Conversation

maier49
Copy link
Contributor

@maier49 maier49 commented Nov 15, 2018

**Type:**feature

The following has been addressed in the PR:

Description:
Adds the concept of "initializers" that can perform sync/async setup before a process is run.
Resolves #90

@agubler
Copy link
Member

agubler commented Nov 30, 2018

Some general thoughts:

  • As this is going to be released into v5.0.0, we can take a look at the existing API. Do you think it would be possible for middleware to support both scenarios, over adding a new concept in initialisers?
  • Would it make more sense to manage the chaining/composition of the middleware internally, so that consumers don't need to use the create*Decorator functions, perhaps just taking an array of middlewares?
  • It would be great if initialisers weren't always treated as async as it means a process with sync commands won't run sync?
  • Do you think that the payload is the only thing that an initialiser will ever need? What about generic scenarios that might require the initialiser to mark some state in the store? isLoading as an example.
  • It looks like there's some duplication across createInitializerDecorator and createCallbackDecorator which can probably be normalised (Possibly not relevant if we change how the middleware is passed)

@maier49
Copy link
Contributor Author

maier49 commented Nov 30, 2018

@agubler

Some general thoughts:

  • As this is going to be released into v5.0.0, we can take a look at the existing API. Do you think it would be possible for middleware to support both scenarios, over adding a new concept in initialisers?

Would something like providing an object for middleware with

{ 'before': {one or more before middlewares}, 'after': {one or more after middlewares} }

Make sense to you? I tried for an API where each middleware was run before and after, by receiving the payload and passing it to a next that returns the result. But it ended up feeling much more complicated to use when mostly you'd want one or the other. It also makes the order that they execute in less obvious.

@matt-gadd
Copy link
Contributor

matt-gadd commented Nov 30, 2018

@agubler

Some general thoughts:

  • As this is going to be released into v5.0.0, we can take a look at the existing API. Do you think it would be possible for middleware to support both scenarios, over adding a new concept in initialisers?

Would something like providing an object for middleware with

{ 'before': {one or more before middlewares}, 'after': {one or more after middlewares} }

Make sense to you? I tried for an API where each middleware was run before and after, by receiving the payload and passing it to a next that returns the result. But it ended up feeling much more complicated to use when mostly you'd want one or the other. It also makes the order that they execute in less obvious.

could we have the middleware itself return the before and after? that way users just use middleware and don't need to know where it has to go. It also means middleware can have a before and after without the user having to specify the middleware twice.

@maier49
Copy link
Contributor Author

maier49 commented Dec 3, 2018

@agubler @matt-gadd

I've made updates to address all your feedback.

Middleware now returns an object with optional before and after properties.

after has the same signature as the old middleware and before gets the payload as well as the store.

Instead of exposing the decorator it's just used internally and all the public facing APIs expect ProcessCallbacks directly.

@agubler agubler added the breaking change Indicates the issue/pull request would result in a breaking change label Dec 10, 2018
@matt-gadd
Copy link
Contributor

@maier49 apologies for the late review on this. this looks really good to me, I just have one question on the ordering of the after calls. Do we think it should be in reverse order like in the PR and why? In my head I can't quite decide what the correct ordering should be, and which is less surprising to the end user.

@matt-gadd matt-gadd changed the title Add initializers Add before and after hooks to store middleware Dec 11, 2018
Copy link
Member

@agubler agubler left a comment

Choose a reason for hiding this comment

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

Super! Approved pending Matt's question!

@maier49
Copy link
Contributor Author

maier49 commented Dec 13, 2018

@matt-gadd The thought process for the order is borrowed from some request hooks as the last interceptor that modifies the request might need to be the first to handle the response, for example for encrypting/decrypting. However, that doesn't really apply the same way to store processes, so I could be convinced either way on the ordering.

@agubler
Copy link
Member

agubler commented Dec 13, 2018

@maier49 I leaning towards the before and after middlewares being executed in the order that they are defined. If I had a process that defined middleware that only executed after the process, i think it would be surprising if they executed in reverse order.

const myProcess = createProcess('example', [ command ], [ afterOne, afterTwo ]);

@maier49
Copy link
Contributor Author

maier49 commented Dec 14, 2018

@agubler That's updated now

@agubler agubler merged commit a791008 into dojo:master Dec 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Indicates the issue/pull request would result in a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants