Proposal: types.hooks / TYPE.withHooks

[xaviergonz]

Right now hooks / snapshot pre/post processing can only be done over models, but what if it could be applied to any kind of type node with something like this:

// all of them are optional
const hooks = {
  preProcessSnapshot(sn) {},
  postProcessSnapshot(sn) {},
  afterCreate(self) {}, // not available in scalar nodes
  afterAttach(self) {},
  beforeDetach(self) {}, // not available in scalar nodes
  beforeDestroy(self) {}
}

// option A
const hookedString = types.hooks('optionalName or will inherit the name', types.string, hooks)

// option B
const hookedString = types.string.withHooks('optionalName or will inherit the name', hooks) // can be chained

With that in place, doing something like a types.optionalNull would be kind of trivial:

function optionalNull<IT extends IAnyType>(type: IT, defaultValueOrFunction: OptionalDefaultValueOrFunction<IT>) {
  const nullToUndefined = types.hooks(types.union(types.null, type), {
    preProcessSnapshot: (sn) => sn === null ? undefined : sn,
    postProcessSnapshot: (sn) => sn === undefined ? null : sn
  })

  return types.optional(nullToUndefined(type), defaultValueOrFunction)

Also this could coexist with the current way until (if?) the hooks inside model actions / .pre-.postProcessSnapshot get deprecated in a future v4

The only con I can see is that the hooks would no longer be able to access "internal state" on the model, e.g.

types.model(...).actions(self => {
  let x = 0;
  return {
    // hooks can access x
  }
})

But for that something like

setState(node, data)
getState(node, data)

could be created

Optionally it could also deprecate types,refinement by adding a validation function to the hooks themselves

Thoughts?

[xaviergonz]

Related issue: https://github.com/mobxjs/mobx-state-tree/issues/947

[mweststrate]

A risk might be performance, previously, all values in MST were backed by the general concept of 'node' that took care of all the householding around values. This was split into scalarnodes and objectnodes, that significantly improved performance; scalarnodes are pretty dumb and used for leaf values (primitives / frozen), and object nodes are still pretty dynamic, being able to perform all the chores around hooks and such. The risk of introducing hooks for scalars could be to loose a bit of those performance improvements.

[k-g-a]

Making hooks as a separate wrapper type is a viable idea in general. To my opinion, types.hooks looks more naturall in MST semantics compared to TYPE.withHooks. Besides "internal state" flaw mentioned above (which I do not consider as a flaw at all) I have the following questions to discuss: 1) Do we really need to expose hooks on ScalarNodes? Taking into account that two of those hooks are forbidden/won't work (silently or not?) - it's harder to teach/explain. 2) Will it hurt performance in noticable way? I'm especially worried about memory consumption as it's still far from acceptable for middle-to-large trees, and MST is actually a tool for making management of such trees easier (simple state/context satisfy small-to-middle projects well nowdays). 3) As an addition to 2nd point - pre/postProcessSnapshot now lives on type (as it's pure), but if we mix those two with the rest of the hooks we will have to either separate them internally or attach pre/postProcessSnapshot to every instance. 4) If we are going to touch hooks, it seems like a good place to introduce afterInitialized (actual name to be discussed) - the one wich fire after createObservalbeInstance done it's work.

I'd suggest to leave hooks for ComplexTypes only (model/map/array) in form of .hooks method:

const myModel = types.model('MyModel', {
  foo: types.string,
 // other props
})
.hooks(() => ({
  preProcessSnapshot(sn) {},
  postProcessSnapshot(sn) {},
  afterCreate(self) {}, 
  afterAttach(self) {},
  beforeDetach(self) {}, 
  beforeDestroy(self) {}
}))

1) It should work same as actions/views/etc. - produce new type via cloneAndEnhance 2) It does not accept self as an argument, instead every hook will recieve self as the only argument. This allows us to

• run .hooks() in type's constructor once (eager); in this scenario we do not even need to pass a delegate to .hooks() - could be plain object;
• run .hooks() only once on first .instantiate() and store result at this.hooks - same as this.properties (lazy); this one creates additional closure, but allows user to have "internal state" between hooks; This will reduce memory footprint and make hooks reusable between types. We could implement both scenarios by simply checking input. 3) As hooks could be collected at type construction time, it would be a good place to run afterInitialized (if it's present). As every hook recieves it's own argument (snapshot for processors, self for others) - we could use it and pass INode or nothing to afterInitialized hook. 4) Hooks are not intended to be exposed on instance, i.e. user can not call myInstance.afterCreate() manually. It's a good motivation to extract reusable code into separate action (and call it inside appropriate hook, if needed).

[k-g-a]

BWT, as we touched the performance: internal EventHandlers added for both scalar and object nodes retain significant amount of memory. For example, on my personal playground of 1 tree with 100 children each having 10 grandchildren (1001 node, every with 2-3 props, 2-3 views, 2-3 actions) its 55Kb, which is about 9-11%: It's shallow size, but the retained one is almost the same as every handler only holds an array which is empty in my case (no reference's).

I mentioned it not to blame anyone (including myself, as I saw the review :)), but to point out that little drops make the mighty ocean: adding several non-significant (in terms of performance) features may sum up into noticable problems.

[luisherranz]

I really like the idea of having hooks separated from actions with their own .hooks(() => ({...})) function.

Two suggestions:

• If self is not sent as an argument, why not turn it into an object, like .props()?:

  .hooks({
  preProcessSnapshot(sn) {},
  postProcessSnapshot(sn) {},
  afterCreate(self) {}, 
  afterAttach(self) {},
  beforeDetach(self) {}, 
  beforeDestroy(self) {}
  })

• Maybe it'd be a good moment to add a beforeCreate or beforeInitialized hook. A lot of people new to MST don't get why afterCreate's don't trigger until the node is observed. That causes a lot of bugs/missunderstandings in their code. I think a new beforeInitialized hook that triggers on model creation, before that node is observed, would solve those issues.

We talked about that with more detail here in the effects issue: https://github.com/mobxjs/mobx-state-tree/issues/867

[xaviergonz]

Thanks all for the input!

After thinking some more about it I think pre/post process snapshots functions should be separated from the other hooks just for exclusively typescript reasons. Right now models have to use a hideous "CustomC, CustomS" template parameters just because they are included with everything else, but if they were in a separate method they could be typed much more easily:

function types.snapshotProcessor<C, S, T, C2 = C, S2 = S>(
    // plus optional new name
    type: IType < C, S, T >,
    processors: {
        preSnapshotProcessor?(sn: C): C2,
        postSnapshotProcessor?(sn: S): S2,
    }
): IType<C2, S2, T>

That would avoid the mess that is needed right now in the typings to make those work (and make the typings easily adapt to any kind of type).

Also, I'd make it so at least the pre/postProcessor hooks can also be used on scalar nodes, since they can become really handy for transformations (for example transform strings to numbers, strings to other strings, null to undefined, etc).

About the performance, I think that can be easily fixed by "proxiing" the current EventHandler methods through some other methods that will create the EventHandlers object lazily if needed.

e.g.

function registerEvent(node, ...) { /*create EventHandlers object on node and call register on it*/ }
function emit(node, ...) { /* if no has no EventHandlers object no-op, else call emit on it */ }

About the other hooks, yeah, I guess they really aren't that useful on scalar nodes and can be added at a later stage (if ever needed), but with the above approach to fix the performance it shouldn't hurt.

@luisherranz If hooks are made a plain object then there's no way for them to share state, e.g.

.hooks({
  afterCreate(self) {
    // I create here something that needs to be disposed on destroy
  }, 
  beforeDestroy(self) {
    // how to access that thing created on afterCreate here?
  }, 
})

while with either of these two solutions it is possible

.hooks(() => {
  let whatToDispose
  afterCreate(self) {
    // whatToDispose = ...;
  }, 
  beforeDestroy(self) {
    // dispose whatToDispose
  }, 
})

.hooks((self) => {
  let whatToDispose
  afterCreate() {
    // whatToDispose = ...;
  }, 
  beforeDestroy() {
    // dispose whatToDispose
  }, 
})

and while it could be argued that there's "addDisposer" for that, we might be loosing some other use cases there.

About if self should go in hooks((self) => or as an argument to the hook itself, I have no strong preferences, but if k-g-a thinks it would be more performant on the function itself it seems fine by me :)

[xaviergonz]

Btw, instead of adding a new hook (before/afterInitialize), what about doing something like this?

.hooks({
  creationMode?: "lazy" | "eager",
  afterCreate(),
  ...
}

Where when creation mode is set to "eager" it will ensure that the node (plus all the subtree to the root) is forcedly created once the whole tree is ready.

Basically its implementation would be to hook to the internal afterCreationFinalization hook and call createObservableInstanceIfNeeded() upon it, which in turn will call afterCreate and afterAttach, but this time it is ensured that if the model is part of the snapshot then they -will- be called.

This could be also how probably effects would be created internally (since effects require a "built" node anyway and most probably you want to ensure that if there's an effect set it will run no matter if the node is first accessed or not)

[luisherranz]

@luisherranz If hooks are made a plain object then there's no way for them to share state

You're right.

Btw, instead of adding a new hook (before/afterInitialize), what about doing something like this? ...

Seems fine to me.

[k-g-a]

After thinking some more about it I think pre/post process snapshots functions should be separated from the other hooks

Seems fine, as it was discussed at #947.

Also, I'd make it so at least the pre/postProcessor hooks can also be used on scalar nodes, since they can become really handy for transformations

ScalarNode's transformation is currently solved by types.custom. Another solution is to process snapshot at parent, which is ObjectNode (ComplexType). It is much more efficient to process all properties of an object within one function call, than to call a function for every property. Also snapshot's processing for scalar nodes may be in conflict with ObjectNode._initialSnapshot - but have to check the code to be more certain about it.

@luisherranz If hooks are made a plain object then there's no way for them to share state, e.g.

As I mentioned before, we could implement both (plainobject/delegate) so user could decide if he needs some local state between hooks or simpler code.

Btw, instead of adding a new hook (before/afterInitialize), what about doing something like this?

I'd prefer to keep things as simple as possible, without giving many options to end-user. Ideally, things ought to "just work", without even knowing whether its lazy or not. It seems like afterCreate is currently associated with creation, so we could move it to ObjectNode's cration and introduce something like afterAccessed/afterUsed (names TBD).

About the performance, I think that can be easily fixed by "proxiing" the current EventHandler methods through some other methods that will create the EventHandlers object lazily if needed.

Laziness does not solve the problem, it jsut defers it. Same as it happens with lazy ObjectNodes: those are created fast, but as soon as you need to iterate all of them (search/filter/sum/etc) - you're hanged for a second and heap is suddenly 100Mb larger than it was.

[xaviergonz]

ScalarNode's transformation is currently solved by types.custom. Another solution is to process snapshot at parent, which is ObjectNode (ComplexType). It is much more efficient to process all properties of an object within one function call, than to call a function for every property.

Just wondering, would it be possible to implement a types.optionalNull based on another type with a custom type? (which reminds me that custom types typings should be fixed to account for the creation type) As for performance, yeah it would be slower, but I guess it is up to the user to decide if they need that extra bit of performance or rather get the convenience :)

I'd prefer to keep things as simple as possible, without giving many options to end-user. ...

Sounds good, though I'm struggling to understand the difference between the proposed "afterInitialized" (is it after the whole tree has been created but before the node has been created?),"beforeInitialized" (is there even an instance available before initialization? is it actually before lazy creation or after lazy creation?), etc.

Laziness does not solve the problem, it jsut defers it.

Hmm, wouldn't in this case? It would not create any EventHandlers object at all if no hook handlers are registered (which probably would be the case for 99% of scalar nodes)

[luisherranz]

As I mentioned before, we could implement both (plainobject/delegate) so user could decide if he needs some local state between hooks or simpler code.

Sorry, I missed that. Perfect solution.

It seems like afterCreate is currently associated with creation, so we could move it to ObjectNode's cration and introduce something like afterAccessed/afterUsed (names TBD).

This would be ideal. afterCreate is probably the best name for newbies. Then, afterCreate can be changed to afterAccessed, afterUsed, afterObserved, afterInitialized, afterInstanciated...

What about afterObserved? Seems clear to me.

[xaviergonz]

Observed makes me think there's a reaction or something attached to the object. afterLazyCreate(self) and afterInitialized(touch)where touch is a function to get self and therefore enforce lazy creation?

I also though of afterTouch(self) but that might be taken in the wrong sense XD

[luisherranz]

I like the touch idea. Why not getSelf?

  afterCreate(getSelf) {
    const self = getSelf();
    // do stuff with the initialized self.
  }

That way you don't need to initialize the node if you don't need it!

[xaviergonz]

Still it would need a way to get when the node is lazily created in case that's what the user want

async afterCreate(getSelf, getLazySelf) {
  // do stuff before node is actually created (akin to beforeInitialize)
  // choose one or none
  const self = await getLazySelf(); // will keep running once self is lazy created (akin to current afterCreate)
  const self = getSelf(); // will create ("access") the node before returning it (akin to some new eager afterCreate)
  // node is ready
}

[xaviergonz]

Actually we could even take a clue from react hooks and manage the whole creation/destroy cycle in a single place

lifecycle({onCreate, onAttach, onFinalize}) {
  // do stuff before node is actually created, but  (akin to beforeInitialize)

  // optional
  onCreate((self) => {
    // node is ready (afterCreate), but not yet attached
    // optionally return a disposer that would be beforeDestroy
    return () => {};
  });

  // optional
  onAttach((self, parent) => {
    // node is ready, it has been attached to a parent (afterAttach)
    // optionally return a disposer that would be beforeDetach
    return () => {};
  });

  // optional
  onFinalize((self, parent?) => {
    // node is ready, it has been created and possibly attached (unless it is a root node) (afterCreationFinalization)
    // this is probably what 99% of the people would use
    // optionally return a disposer that would be also beforeDestroy
    return () => {};
  });

  return 'lazy' | 'eager'; // return creation (touch) mode, eager or lazy
  // in eager mode the node will be "accessed" as soon as the root node is finalized
}

Also sharing state should be easier

[Amareis]

lifecycle thing completely gets rid of the effects! Is multiple calls of hooks are allowed?

[xaviergonz]

Yeah, lifecycle chaining should be possible. Effects could be modeled over hooks indeed, but still I see them as a valuable addition.

[xaviergonz]

In the end the lifecycle is very close to hooks, so might as well be...

// call as function or with a plain obj
hooks(() => {
  // do stuff before node is actually created, but  (akin to beforeInitialize)

  return {
    // all is optional
    onCreate((self) => {
      // node is ready (afterCreate), but not yet attached
      // optionally return a disposer that would be beforeDestroy
      return () => {};
    }),

    onAttach((self, parent) => {
      // node is ready, it has been attached to a parent (afterAttach)
      // optionally return a disposer that would be beforeDetach
      return () => {};
    }),

    // optional
    onFinalize((self, parent?) => {
      // node is ready, it has been created and possibly attached (unless it is a root node) (afterCreationFinalization)
      // this is probably what 99% of the people would use
      // optionally return a disposer that would be also beforeDestroy
      return () => {};
    }),

    // return creation (touch) mode, eager or lazy
    // in eager mode the node will be "accessed" as soon as the root node is finalized
    creationMode: 'lazy' | 'eager'
  };
});

if multiple hook calls are chained they all will be called in order, where creation mode will be lazy only if all of them are set to lazy

[xaviergonz]

@mweststrate @k-g-a any thoughts?

[mweststrate]

Really like where this is going. Not sure about the 'best' api yet. A consideration: I think hooks and actions quite often want to close over the same state, so we have to make sure .extends supports the same syntax as well. (add which point we could wonder whether we shouldn't always to use .extends instead of calling .views | .actions | .hooks directly 😅. (It might perform slightly better as well, since there would less closures per model type)

[xaviergonz]

The problem I see with hooks inside extend is that it currently gives you access to self, so there would be no way to make a beforeInitialize or any optimizations if a plain object is used instead to declare the hooks.

Also extends kind of suffers from the issue that its typings kind of allow you to easily skip computed views. e.g.

// good
.views(...) // someView here
.actions(...) // some action that uses someView either as this.someView or self.someView, which is a computed

vs

// bad
.extend(self => {
  const views = {} // someView here
  const actions = {} // TS makes you use the view as views.someView, which is not a computed, since self.someView cannot be typed yet
  return {views, actions}
})

so maybe it could be something like

.localState({
  // here stuff for initial local state, TS would use this for types
  // can contain shared methods, or plain variables...
  // if used more than once then the type of local state is the & of the previous one and the next one
  foo: "bar"
})
.volatile((self, localState) => { ... })
.views((self, localState) => { ... })
.actions((self, localState) => {
  localState.foo = "whatever";
})
.hooks((localState) => as on the other post)
.extend((self, localState) => {}) // or eventually deprecate extend so TS users don't fall into those gotchas mentioned above

Of course if the user doesn't want to use localState at all then he'd just not use the parameter and omit it, so it would be backwards compatible

[xaviergonz]

Although now that I think about it, since views is an object returned in extends, it could be patched so its properties are substituted by their computed versions, and same with the actions, volatile, etc.

If that's done then using extends exclusively should be OK (except for the part where it requires self, so no beforeInitialize, but then again I'm not sure how useful beforeInitialize would be anyway).

[k-g-a]

@mweststrate @k-g-a any thoughts?

Looks good to me. Do I get it right, that you just omitted beforeDetach/beforeDestroy from example above, but those are supposed to be present? )

[xaviergonz]

Before detach and before destroy would become optional disposers returned from the initiators as shown in the example actually, but that can be switched back easily if you think discrete events are better

[k-g-a]

Before detach and before destroy would become optional disposers returned from the initiators as shown in the example actually, but that can be switched back easily if you think discrete events are better

So the rule would be "a function returned from the hook will be called at corresponding 'closing' step". My argumants against are: sounds like a magic rule, seems complex to teach, differs from current approach.

[d4rky-pl]

Is there any chance this discussion will be revived or are custom type hooks dead?

Asking because I can't think of a way of having a type that refreshes its value automatically based on another observable (think: date time type that needs to update the timezone) without traversing entire tree and doing it manually and that feels icky :)