From f4d9d710b246e11741a42c3a4d89fceff8339f08 Mon Sep 17 00:00:00 2001 From: Robin Fehr Date: Tue, 13 Feb 2018 00:22:23 +0300 Subject: [PATCH 1/3] toggle hooks in middleware --- API.md | 6 +- README.md | 4 +- packages/mobx-state-tree/src/core/action.ts | 59 ++++++++++++----- .../src/core/node/object-node.ts | 14 +++- .../src/types/complex-types/model.ts | 21 +++--- packages/mst-middlewares/src/undo-manager.ts | 2 +- .../mst-middlewares/test/TimeTraveller.ts | 2 +- packages/mst-middlewares/test/custom.ts | 65 +++++++++++++++++++ 8 files changed, 139 insertions(+), 34 deletions(-) create mode 100644 packages/mst-middlewares/test/custom.ts diff --git a/API.md b/API.md index 0d74f9a6b..2a41b2184 100644 --- a/API.md +++ b/API.md @@ -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. + Returns **IDisposer** @@ -207,7 +209,7 @@ Binds middleware to a specific action **Parameters** -- `middleware` **IMiddlewareHandler** +- `handler` **IMiddlewareHandler** - `fn` - `Function` } fn diff --git a/README.md b/README.md index 1af8b640e..91ca96b38 100644 --- a/README.md +++ b/README.md @@ -906,13 +906,13 @@ See the [full API docs](API.md) for more details. | signature | | | ---- | --- | | [`addDisposer(node, () => void)`](API.md#adddisposer) | Function to be invoked whenever the target node is to be destroyed | -| [`addMiddleware(node, middleware: (actionDescription, next) => any)`](API.md#addmiddleware) | Attaches middleware to a node. See [middleware](docs/middleware.md). Returns disposer. | +| [`addMiddleware(node, middleware: (actionDescription, next) => any, includeHooks)`](API.md#addmiddleware) | Attaches middleware to a node. See [middleware](docs/middleware.md). Returns disposer. | | [`applyAction(node, actionDescription)`](API.md#applyaction) | Replays an action on the targeted node | | [`applyPatch(node, jsonPatch)`](API.md#applypatch) | Applies a JSON patch, or array of patches, to a node in the tree | | [`applySnapshot(node, snapshot)`](API.md#applysnapshot) | Updates a node with the given snapshot | | [`createActionTrackingMiddleware`](API.md#createactiontrackingmiddleware) | Utility to make writing middleware that tracks async actions less cumbersome | | [`clone(node, keepEnvironment?: true \| false \| newEnvironment)`](API.md#clone) | Creates a full clone of the given node. By default preserves the same environment | -| [`decorate(middleware, function)`](API.md#decorate) | Attaches middleware to a specific action (or flow) | +| [`decorate(handler, function)`](API.md#decorate) | Attaches middleware to a specific action (or flow) | | [`destroy(node)`](API.md#destroy) | Kills `node`, making it unusable. Removes it from any parent in the process | | [`detach(node)`](API.md#detach) | Removes `node` from its current parent, and lets it live on as standalone tree | | [`flow(generator)`](API.md#flow) | creates an asynchronous flow based on a generator function | diff --git a/packages/mobx-state-tree/src/core/action.ts b/packages/mobx-state-tree/src/core/action.ts index ae748b878..8aea9808b 100644 --- a/packages/mobx-state-tree/src/core/action.ts +++ b/packages/mobx-state-tree/src/core/action.ts @@ -7,7 +7,8 @@ import { IDisposer, getRoot, EMPTY_ARRAY, - ObjectNode + ObjectNode, + HookNames } from "../internal" export type IMiddlewareEventType = @@ -30,6 +31,10 @@ export type IMiddlewareEvent = { args: any[] } +export type IMiddleware = { + handler: IMiddlewareHandler + includeHooks: boolean +} export type IMiddlewareHandler = ( actionCall: IMiddlewareEvent, next: (actionCall: IMiddlewareEvent) => any @@ -96,7 +101,11 @@ export function createActionInvoker( * @param {(action: IRawActionCall, next: (call: IRawActionCall) => any) => any} middleware * @returns {IDisposer} */ -export function addMiddleware(target: IStateTreeNode, middleware: IMiddlewareHandler): IDisposer { +export function addMiddleware( + target: IStateTreeNode, + middleware: IMiddlewareHandler, + includeHooks: boolean = true +): IDisposer { const node = getStateTreeNode(target) if (process.env.NODE_ENV !== "production") { if (!node.isProtectionEnabled) @@ -104,7 +113,7 @@ export function addMiddleware(target: IStateTreeNode, middleware: IMiddlewareHan "It is recommended to protect the state tree before attaching action middleware, as otherwise it cannot be guaranteed that all changes are passed through middleware. See `protect`" ) } - return node.addMiddleWare(middleware) + return node.addMiddleWare(middleware, includeHooks) } export function decorate(middleware: IMiddlewareHandler, fn: T): T @@ -126,41 +135,61 @@ export function decorate(middleware: IMiddlewareHandler, fn: * * @export * @template T - * @param {IMiddlewareHandler} middleware + * @param {IMiddlewareHandler} handler * @param Function} fn * @returns the original function */ -export function decorate(middleware: IMiddlewareHandler, fn: any) { +export function decorate(handler: IMiddlewareHandler, fn: any) { + const middleware: IMiddleware = { handler, includeHooks: true } if (fn.$mst_middleware) fn.$mst_middleware.push(middleware) else fn.$mst_middleware = [middleware] return fn } -function collectMiddlewareHandlers( +function collectMiddlewares( node: ObjectNode, baseCall: IMiddlewareEvent, fn: Function -): IMiddlewareHandler[] { - let handlers: IMiddlewareHandler[] = (fn as any).$mst_middleware || EMPTY_ARRAY +): IMiddleware[] { + let middlewares: IMiddleware[] = (fn as any).$mst_middleware || EMPTY_ARRAY let n: ObjectNode | null = node // Find all middlewares. Optimization: cache this? while (n) { - if (n.middlewares) handlers = handlers.concat(n.middlewares) + if (n.middlewares) middlewares = middlewares.concat(n.middlewares) n = n.parent } - return handlers + return middlewares } function runMiddleWares(node: ObjectNode, baseCall: IMiddlewareEvent, originalFn: Function): any { - const handlers = collectMiddlewareHandlers(node, baseCall, originalFn) + const middlewares = collectMiddlewares(node, baseCall, originalFn) // Short circuit - if (!handlers.length) return mobxAction(originalFn).apply(null, baseCall.args) + if (!middlewares.length) return mobxAction(originalFn).apply(null, baseCall.args) let index = 0 function runNextMiddleware(call: IMiddlewareEvent): any { - const handler = handlers[index++] - if (handler) return handler(call, runNextMiddleware) - else return mobxAction(originalFn).apply(null, baseCall.args) + const middleware = middlewares[index++] + const handler = middleware && middleware.handler + const invokeHandler = () => { + const next = handler(call, runNextMiddleware) + if (!next && index < middlewares.length && process.env.NODE_ENV !== "production") { + const node = getStateTreeNode(call.tree) + fail( + `The next() callback within a middleware for the action: "${call.name}" on the node: ${node + .type.name} wasn't invoked.` + ) + } + return next + } + + if (handler && middleware.includeHooks) { + return invokeHandler() + } else if (handler && !middleware.includeHooks) { + if ((HookNames as any)[call.name]) return runNextMiddleware(call) + return invokeHandler() + } else { + return mobxAction(originalFn).apply(null, baseCall.args) + } } return runNextMiddleware(baseCall) } diff --git a/packages/mobx-state-tree/src/core/node/object-node.ts b/packages/mobx-state-tree/src/core/node/object-node.ts index 24cbc7218..c8d0e764e 100644 --- a/packages/mobx-state-tree/src/core/node/object-node.ts +++ b/packages/mobx-state-tree/src/core/node/object-node.ts @@ -15,6 +15,7 @@ import { registerEventHandler, addReadOnlyProp, walk, + IMiddleware, IMiddlewareHandler, createActionInvoker, NodeLifeCycle, @@ -47,7 +48,7 @@ export class ObjectNode implements INode { protected _autoUnbox = true // unboxing is disabled when reading child nodes state = NodeLifeCycle.INITIALIZING - middlewares = EMPTY_ARRAY as IMiddlewareHandler[] + middlewares = EMPTY_ARRAY as IMiddleware[] private snapshotSubscribers: ((snapshot: any) => void)[] private patchSubscribers: ((patch: IJsonPatch, reversePatch: IJsonPatch) => void)[] private disposers: (() => void)[] @@ -404,8 +405,15 @@ export class ObjectNode implements INode { this.disposers.unshift(disposer) } - addMiddleWare(handler: IMiddlewareHandler) { - return registerEventHandler(this.middlewares, handler) + removeMiddleware(handler: IMiddlewareHandler) { + this.middlewares = this.middlewares.filter(middleware => middleware.handler !== handler) + } + + addMiddleWare(handler: IMiddlewareHandler, includeHooks: boolean = true) { + this.middlewares.push({ handler, includeHooks }) + return () => { + this.removeMiddleware(handler) + } } applyPatchLocally(subpath: string, patch: IJsonPatch): void { diff --git a/packages/mobx-state-tree/src/types/complex-types/model.ts b/packages/mobx-state-tree/src/types/complex-types/model.ts index 2b35decff..27e83ef44 100644 --- a/packages/mobx-state-tree/src/types/complex-types/model.ts +++ b/packages/mobx-state-tree/src/types/complex-types/model.ts @@ -42,12 +42,12 @@ import { const PRE_PROCESS_SNAPSHOT = "preProcessSnapshot" -const HOOK_NAMES = { - afterCreate: "afterCreate", - afterAttach: "afterAttach", - postProcessSnapshot: "postProcessSnapshot", - beforeDetach: "beforeDetach", - beforeDestroy: "beforeDestroy" +export enum HookNames { + afterCreate = "afterCreate", + afterAttach = "afterAttach", + postProcessSnapshot = "postProcessSnapshot", + beforeDetach = "beforeDetach", + beforeDestroy = "beforeDestroy" } function objectTypeToString(this: any) { @@ -72,7 +72,7 @@ function toPropertiesObject(properties: IModelProperties): { [K in keyof T return Object.keys(properties).reduce( (properties, key) => { // warn if user intended a HOOK - if (key in HOOK_NAMES) + if (key in HookNames) return fail( `Hook '${key}' was defined as property. Hooks should be defined as part of the actions` ) @@ -169,9 +169,9 @@ export class ModelType extends ComplexType implements IModelType specializedAction(baseAction(snapshot)) else action = function() { @@ -381,8 +381,9 @@ export class ModelType extends ComplexType implements IModelType don't throw here but set a global var instead + if (call.name === "postProcessSnapshot") error = Error("hook in middleware") + return next(call) +} + +const TestModel = types + .model("Test1", { + z: 1 + }) + .actions(self => { + return { + inc(x) { + self.z += x + return self.z + }, + postProcessSnapshot(snapshot) { + return snapshot + } + } + }) + +test("next() omitted within middleware", t => { + const m = TestModel.create() + addMiddleware(m, customMiddleware1) + addMiddleware(m, customMiddleware2) + let thrownError: any = null + try { + m.inc(1) + } catch (e) { + thrownError = e + } + t.is(!!thrownError, true) +}) + +test("middleware should be invoked on hooks", t => { + const m = TestModel.create() + error = null + addMiddleware(m, noHooksMiddleware) + m.inc(1) + t.is(!!error, true) +}) + +test("middleware should not be invoked on hooks", t => { + const m = TestModel.create() + error = null + addMiddleware(m, noHooksMiddleware, false) + m.inc(1) + t.is(!error, true) +}) From 70ccf35ccda4ca40d849e44822e7ff3cf789978f Mon Sep 17 00:00:00 2001 From: Rob Date: Sat, 24 Feb 2018 23:16:38 +0300 Subject: [PATCH 2/3] information about hooks to the API.md --- API.md | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/API.md b/API.md index 2a41b2184..5fe8c5266 100644 --- a/API.md +++ b/API.md @@ -33,6 +33,7 @@ _This reference guide lists all methods exposed by MST. Contributions like lingu - [getSnapshot](#getsnapshot) - [getType](#gettype) - [hasParent](#hasparent) +- [hooks](#hooks) - [Identifier](#identifier) - [IdentifierCache](#identifiercache) - [isAlive](#isalive) @@ -125,7 +126,9 @@ For more details, see the [middleware docs](docs/middleware.md) - `target` **IStateTreeNode** - `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. +- `includeHooks` **([boolean](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Boolean) | any)** indicates whether the [hooks](#hooks) should be piped to the middleware. + +See [hooks](#hooks) for more information. Returns **IDisposer** @@ -387,6 +390,27 @@ Given a model instance, returns `true` if the object has a parent, that is, is p Returns **[boolean](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Boolean)** +## hooks + +The current action based LifeCycle hooks for [types.model](#types.model). + +```typescript +enum HookNames { + afterCreate = "afterCreate", + afterAttach = "afterAttach", + postProcessSnapshot = "postProcessSnapshot", + beforeDetach = "beforeDetach", + beforeDestroy = "beforeDestroy" +} +``` + +Note: +Unlike the other hooks, `preProcessSnapshot` is not created as part of the actions initializer, but directly on the type. +`preProcessSnapshot` is currently not piped to middlewares. + +For more details on middlewares, see the [middleware docs](docs/middleware.md) + + ## Identifier ## IdentifierCache From dffd1f0580e08b3851fb821bb6fdd8f183c8f077 Mon Sep 17 00:00:00 2001 From: Rob Date: Sun, 25 Feb 2018 15:17:28 +0300 Subject: [PATCH 3/3] remove check to detect non invoked/returned callbacks within middlewares --- packages/mobx-state-tree/src/core/action.ts | 7 ------- 1 file changed, 7 deletions(-) diff --git a/packages/mobx-state-tree/src/core/action.ts b/packages/mobx-state-tree/src/core/action.ts index 8aea9808b..df2d33950 100644 --- a/packages/mobx-state-tree/src/core/action.ts +++ b/packages/mobx-state-tree/src/core/action.ts @@ -172,13 +172,6 @@ function runMiddleWares(node: ObjectNode, baseCall: IMiddlewareEvent, originalFn const handler = middleware && middleware.handler const invokeHandler = () => { const next = handler(call, runNextMiddleware) - if (!next && index < middlewares.length && process.env.NODE_ENV !== "production") { - const node = getStateTreeNode(call.tree) - fail( - `The next() callback within a middleware for the action: "${call.name}" on the node: ${node - .type.name} wasn't invoked.` - ) - } return next }