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

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

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 maier49 force-pushed the maier49:add-initializers branch from af69dc6 to 3b0edb9 Dec 3, 2018

@maier49

This comment has been minimized.

Copy link
Contributor Author

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.

@matt-gadd

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

@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

@agubler
Copy link
Member

left a comment

Super! Approved pending Matt's question!

@maier49

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

commented Dec 14, 2018

@agubler That's updated now

@agubler agubler merged commit a791008 into dojo:master Dec 14, 2018

4 checks passed

codecov/patch 100% of diff hit (target 97.15%)
Details
codecov/project 97.16% (+0.01%) compared to de9d069
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.