toggle hooks in middleware #653
Conversation
| const next = handler(call, runNextMiddleware) | ||
| if (!next && index < middlewares.length && process.env.NODE_ENV !== "production") { | ||
| const node = getStateTreeNode(call.tree) | ||
| fail( |
There was a problem hiding this comment.
I'm not sure about this check, there are cases where it is fine that next() isn't called, for example if you have middleware that prevents modifications, or only applies it to a copy, etc. I had a few cases before. I am aware it is pretty tricky if you forget it in your middleware, but I think that is an inherent risk about writing your own middleware and hopefully docs help preventing mistakes there :)
There was a problem hiding this comment.
fair point about aborting the middleware queue.
what about returning false if it the abort was intended and only throw on undefined?
or something similar more expressive
There was a problem hiding this comment.
e.g. an abort() next to next()
API.md
Outdated
| @@ -124,7 +124,9 @@ For more details, see the [middleware docs](docs/middleware.md) | |||
| **Parameters** | |||
|
|
|||
| - `target` **IStateTreeNode** | |||
| - `middleware` | |||
| - `middleware` **IMiddleware** | |||
| - `includeHooks` **([boolean](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Boolean) | any)** indicates whether the hooks should be piped to the middleware. | |||
There was a problem hiding this comment.
elobarote; that is, should the middleware be run for built in methods like applySnapshot`
|
Merged! |
we might wan't to fine grain it so we can choose which hooks.
the default is 'included' to not break current implementations.