Mobx-state-tree: Feature - effects

[AjaxSolutions]

I'm new to MST, so forgive me if my question has an obvious answer.

Vue has computed properties similar to MST's views, but it also has watch properties.

If you want to learn about watch properties, here's the Vue doc:
https://vuejs.org/v2/guide/computed.html#Computed-vs-Watched-Property

Also, watch this video, scroll to the 28:00 mark:
https://youtu.be/UHmFXRp0JDU?t=1691

Would it be possible to add watch properties to MST?

[mattruby]

See:
https://mobx.js.org/refguide/reaction.html
and
https://mobx.js.org/refguide/autorun.html

On Tue, Jun 12, 2018 at 3:41 PM, Les Szklanny [e-mail]
wrote:

I'm new to MST, so forgive me if my question has an obvious answer.

Vue has computed properties similar to MST's views, but it also has watch
properties.

If you want to learn about watch properties, here's the Vue doc:
https://vuejs.org/v2/guide/computed.html#Computed-vs-Watched-Property

Also, watch this video, scroll to the 28:00 mark:
https://youtu.be/UHmFXRp0JDU?t=1691

Would it be possible to add watch properties to MST?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/mobxjs/mobx-state-tree/issues/867, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAIrcnkC_h_PNUpYn1Sxqj72a6BWPiDFks5t8CeGgaJpZM4UlHtu
.

--
-Matt Ruby-
[email protected]

[AjaxSolutions]

Thanks. I think adding reactions/watchers to MST alongside views and actions would be a good feature even though this is already available in MobX.

[mweststrate]

I think this is a pretty cool idea, because it would save some boilerplate around disposers.

[mweststrate]

Big question is when should those reactions be setup. afterAttach or afterCreate? AfterAttach is not always triggered (not for roots) or multiple times (when moving nodes, which is not too common), afterCreate however does not give access to parents yet

[jayarjo]

I was wondering about the same thing recently. I will see if I can PR anything meaningful here.

[jayarjo]

I'm afraid I will need a hint on why something like this might not be a good idea.

[jayarjo]

It is not clear how to behave if the property to watch is observable array or map. What exactly should the listener receive? newValue and oldValue won't be any much meaningful in such context.

[jayarjo]

I checked what Vue does in such cases and I think they still supply newValue and oldValue despite of them being useless, as they reference the same object.

Note: when mutating (rather than replacing) an Object or an Array, the old value will be the same as new value because they reference the same Object/Array. Vue doesn’t keep a copy of the pre-mutate value.

Although not sure if we can do better and keep the immediate simplicity at the same time. @AjaxSolutions what would you expect for example?

[jayarjo]

Maybe it has sense to provide full change object instead of newValue/oldValue pair. Then users could use spread operators to extract only required properties.

[jayarjo]

Until someone suggests a better solution I thought it could simply give the full change object to the listener, unless listener explicitly requests two or more arguments (correspondingly newValue, oldValue and an optional changeType).

@mweststrate I guess your comment on https://github.com/mobxjs/mobx-state-tree/commit/1dfe7f4d691b2febe7d9c1fc5ff52157d22d39e2 branch is still very appreciated or anyones with comparable insight into the inner wirings of MST, 'cause I should have overlooked whole lot (types is surely a definite one that I missed).

[xaviergonz]

why not just offer inside self for model types a reaction/autorun method that will add the disposer to some registry that gets disposed on destroy and let the user decide when to call it? (usually inside afterattach or aftercreate)

[jayarjo]

@xaviergonz what would be the benefit comparing to the solution in the draft? Also don't we have it already through reaction/autorun imports and addDisposer method - one could invoke these from whatever place he likes.

[AjaxSolutions]

If possible, I'd like this feature to work exactly as in Vue, which IMO is easier to use than autorun or reaction.

See the options provided by Vue's watchers.
https://vuejs.org/v2/api/#watch

This should cover 80% of use cases. For more complex requirements you can still use autorun or reaction.

See this tweet by the Evan You - the Vue creator. I agree with him that Vue is easier to use than React in part because of watch properties.

https://twitter.com/youyuxi/status/736939734900047874?lang=en

[Amareis]

In mobx-react there is disposeOnUnmount decorator now, which handles similar issue. But in react's observer components there is no problem with moment when start watching - at component creation you already have the mst tree (or mobx store).

How @mweststrate says, mst node cannot just start watching (I prefer "reacting" word, it closer to mobx universe) at contruction or at afterCreate/afterAttach - because, in general, it can reacts to whole tree, includes parents and neighbors, so node should wait for full ready tree, and, currently, there is simply no way to know about (because it depends on user intentions).

So, what we can do with it? In mobx there is three main concepts: observables, computed and, in end (or in begin?.., huh interesting question), reactions. First two also exists in mst, but for reactions there is no any analogs - we just uses plain mobx reactions, as recommended in docs.

Wait! If mst is living tree, as it described in docs, why it cannot reacts to anything? It's more like a potted tree which dies quickly without the gardener! ;)

I think, lack of side effects tools (I prefer "main effects" word, because, usually, all interesting things are happens here) is big gap for state management library. If we look at redux, there is redux-saga - side effects library. In our main application, which now in migration from redux to mst, we heavily use sagas for big part of business logic, because pure and synchronous way of redux simply doesn't fit the real world.

Of course, bunch of autoruns and reactions in some file can handle all of these effects, but why we cannot embed it into mst? There is pretty straigtforward way to do it:

1. Add effects chainable method, just as actions or views. Effect is function which subscribes to something and return itself disposer - right, just as autorun, but also it can be just some addEventListener/removeEventListener pairs or something else. Of course, effects should use actions for tree changing.

types.model('Todo', {
  id: types.identifier,
  title: types.string,
  finished: false,
}).actions(self => ({
  update: (newTodo) => Object.assign(self, newTodo),
})).effects(self => ({
  notifyBackend: () => autorun(() => api.setTodoFinished(self.id, self.finished)),
  listenBackend: () => {
    api.listenTodoChanges(self.id, newTodo => self.update(newTodo))
    return () => api.stopListenTodoChanges(self.id) //just an example, there is can be some removeEventListener or anything disposer-like
}))

1. Add affect(node) function in mst core, which recursively run all effects in tree and marks node as affected recursively.
2. Add unaffect(node) function in mst core, which dispose all effects in tree and remove affected mark recursively.
3. Add isAffected(node) function in mst core (you guessed, it returns true if node is affected!).
4. Internally, we always unaffect node before destroying.
5. Node is not unaffected on detach - user can do it himself if needed.
6. Attaching not affected node to affected parent is special case (when user make something like self.todos.push({title: 'New todo'}), if todos is affected, new todo will constructed and it's not affected yet, but it should be affected after attach in most use cases). In such case node will be affected and if user wants to avoid this, he should explicit invokes unaffect(self) in afterAttach hook (effects will not even started in that case).

I think this concept is simple enough for understanding, implementing and maintaining. What are you thinks?
@mweststrate @AjaxSolutions @jayarjo @xaviergonz

[Amareis]

Also, affect and unaffect can use optional second argument, which disables recursion. For example, on node destroying there is no mean to recursive unaffect - child nodes just will invoke unaffect at own destroying.

[Amareis]

So, can anybody review my request? I can make a PR, but there is recommendation in readme - discuss extensive changes in issues first.

Some examples.
Currently, I have that code (it's with classy-mst, but it doesn't important):

class ItemCode extends shim(ItemData) {
    subs: any = []

    @action
    afterAttach() {
        this.subs = [
            app.api.chatUpdated(c => c.id === this.id, this.merge),
            app.api.messageCreated(m => m.chat === this.id, m => this.insertMessages([m])),
            app.api.todoCreated(t => t.chat === this.id, this.addTodo),
        ]
    }

    @action
    beforeDestroy() {
        this.subs.forEach((unsub: Function) => unsub())
    }
    //...
}

With some sort of autodisposed effects or watchers (which is special case of effect) this code can be rewritten to:

class ItemCode extends shim(ItemData) {
    @effect
    subscribe() {
        return [
            app.api.chatUpdated(c => c.id === this.id, this.merge),
            app.api.messageCreated(m => m.chat === this.id, m => this.insertMessages([m])),
            app.api.todoCreated(t => t.chat === this.id, this.addTodo),
        ]
    }
    //...
}

Another effects, which I manage manually now, also can be pulled out to different, explicitly marked as effect, methods, so code will be more transparent and antifragile.

[mweststrate]

To fix the 'at what point in the lifecycle' problem, would it be fine to set up effects on the next tick?

(also, minor correction on the above reasoning: side effects are first class in MST, through for example actions and flows, just _automatic_ side effects are generalized into their own API)

[Amareis]

Next tick after creating? Yeah, it should works. In that case we don't need to explicit call affect, only unaffect node after creation if we don't want to start effects automatically.
So,
Todo.create({title}) - effects are started at next event loop.
let todo = Todo.create({title}); unaffect(todo) - effects will not started until explicit affect call. Same behaviour if user invokes unaffect(self) in afterCreate or, if node attached immediatly after creating, in afterAttach(self).
Should we add separated willAffected function to core for detect planned to affect nodes?

[xaviergonz]

I like the idea of effects, why the need for names though? it could just be an array (or both object and array could be supported)

[xaviergonz]

About the implementation, why not just hook it into the afterCreate / beforeDestroy events (or afterAttach / beforeDeatch) and if somebody wants something more custom let him do it hooking it himself through any of the other hooks?

With an options argument to effects declaring when you want them to be run

.effects(...., { mode: 'onCreate' | 'onAttach' }) // defaulting to onCreate

if the user wants different effects for different lifecycles he can just call effect twice with different options

[xaviergonz]

Then again I'm not sure how custom events are gonna get along with lazy instantiation.

[xaviergonz]

Just wondering, why do effects need to be run on the next tick? can't they just run right after "afterCreate"/"afterAttach" are finished?

[Amareis]

can't they just run right after "afterCreate"/"afterAttach" are finished?

I think you're right, but also there is should be a third mode, manually or something like it, which runs only at explicit user intention (direct call or affect at node).
Also, effect should be a functions which returns disposer. In that way user can runs/stops they just as simple functions and it will be easier to stop/rerun all effects on node.

[xaviergonz]

What's the use case for manual triggering?

As for choosing if effects should be run or not it could be done on create
.create(..., {runEffects: true}) // true (or maybe false?) by default

[Amareis]

Ok, I think third mode can be added after some battle testing, if needed. runEffect should be true by default, because create can be called implicitly at snapshot converting.

[xaviergonz]

Ok, a couple of points, I'd totally not have a mode (create or after attach), and this is why:

runEffect for implicit conversions could be inherited from the value when it was used on create of the parent
e..g

const m = X.create(..., {runEffects: true}}/ force run on itself and children that do not specify a preference
  m.child = {...} or Child.create() // effects will run (inherited)
  m.child = Child.create(..., {runEffects: true}) // will run for itself  and children that do not specify a preference
  m.child = Child.create(..., {runEffects: false}) // won't run on itself  and children that do not specify a preference

const m = X..create(..., {runEffects: false}) // force not run on itself and children
  m.child = {...} or Child.create() // effects won't run (inherited)
  m.child = Child.create(..., {runEffects = true}) // will run for itself  and children that do not specify a preference
  m.child = Child.create(..., {runEffects = false}) // won't run on itself  and children that do not specify a preference

and if there's no runEffects set and no parent the it will assume undefined

why undefined?

const parent = Parent.create(..., {runEffects: true}) // effects will be run as requested, in this case as part of "afterCreate"
const child = Child.create(...) // inherit, but we don't know if it will be a root node or not, but since the default is undefined we won't run them right now
parent.setChild(child) // aha, now we know what the inheritance asks of it, so we run the effects (albeit admittedly as part of an "attach" event

summarizing:

• runEffects: true - run them on creation (after afterCreate is done), dispose on beforeDestroy
• runEffects: false - do not ever run them
• runEffects: undefined (default) - on afterAttach... (dispose on beforeDetach)
• 
  
  • if the nearest parent with runEffect !== undefined is true - run them
  
  
• 
  
  • if the nearest parent with runEffect !== undefined is false - do not run them
  
  
• 
  
  • if there's no nearest parent with runEffect set - don't run them
  
  
• 
  
  • never attached - will wait until it is attached of course to decide
  
  

The good thing is that this would be possible:

• create a node C with runEffects = undefined
• C gets attached to P1, which has run effects set to true
• C runs its effects upon attachment
• C gets detached, effects are disposed
• C now gets attached to P2, which has run effects set to false
• no C effects are run as per parent "config"
• C now gets attached to P3, which has run effects set to true
• C runs its effects once more upon attachment
• P2 tree dies, C is detached, C disposes of its effects

Basically it is done for you and you don't have to think about it. The usual would just be to set runEffects to true upon creating the root node of the store.

I hope that makes sense.

That being said, wouldn't the api be more clear like this?

types.model(...)
.reaction((self) => self.id, (self, newValue) => { do whatever }, reactionOptions?)
.autorun((self) => { do whatever }, autorunOptions?)
.customEffect((self) => { return a disposer }, options?)

that'd be more akin to watch properties too

Also I'd also maybe add those methods to any complex type (map, array, model, etc), not only models (but of course not to simple types such as string), but that could be left for the future since those types don't have hooks right now.

And I guess the clone method would need options with runEffects too :)

[Amareis]

Instead of customEffect there is can be just effect and reaction, autorun etc (onAction, may be, as suggested in #1056?) it's just sugar for effect. Also, we need functions for start/stop effects manually (what if we detach node and want to effects don't stop, or stop effects on some subtree?) and one for detecting if effects are running.

[k-g-a]

I do incline to the latter proposal with API clearly mirroring mobx's one - it's dead simple and gives reasonable flexibility.
Single setting (runEffects) looks acceptable and I can understand it's use-cases, although same result could be achieved by proper composition.
Best place to run effects seems to be ObjectNode.finalizeCreation. We could introduce afterFinalized() hook (as the last meaningful action of the method) and use it internally.

[k-g-a]

Can't think of any use cases for starting/stopping effects manually. It would be great to have an example.

[xaviergonz]

I guess customEffect could be just effect yes

@k-g-a although same result could be achieved by proper composition.

what do you mean?

[Amareis]

Anyway we need some internal function to start/stop effects. Why don't give it to users? I can imagine what some user need to detach some existing subtree and run effects on it again, or disable effects in subtree under some circumstances.

[luisherranz]

We're using some mobx effects (autorun, reaction...) in afterCreate but when we upgraded to MST3 we found some of them weren't triggered on creation but on access, due to the lazy mode introduced in MST3. That introduced some bugs and we had to "force" the access to launch those afterCreates.

Don't get me wrong, lazy mode is awesome (out .create() now takes 6 times less!) but it doesn't play nice with effects. Maybe if runEffects is true it should disable lazy mode? Or maybe a new option should be added, lazy, true by default so devs can control it?

By the way, following this reasoning, afterCreate in lazy mode is a bit confusing. Maybe afterAccess or afterFirstAccess would be easier to understand.

[jayarjo]

Ehm... I still miss a point sorry. Has anyone here looked at this branch? And if yes, could you please explain to me:

1. why can't we simply attach reactions when node is instantiated, rather than on afterCreate/afterAttach? my understanding was that all children of the node will be instantiated bby that moment.
2. node is monitored only if reactions object is not empty and disposers are added via addDisposer to the internal cache, that is auto-purged when node is destroyed, so that's good?
3. also suggested api provides access not only to new, but also to the old value of the monitored observable or computable property, isn't it handy?

[Amareis]

Effects is more general than watchers, so we can implement watchers with access to old values on effects base. Dedicated effects also removes some boilerplate and code will be more decoupled (effects will not explodes afterCreate , but placed at own section).

[xaviergonz]

@jayarjo it is certainly more akin to watch props, but what if you want to react to a child component prop from the parent?

For example, say that you want to trigger a reaction inside the todoStore when a todo is added (like making a rest call to save it on the server)

Then also, you might want to disable that API call when you are inside tests (although I guess a way around this would be to use getEnv to avoid those)

As for attaching reactions when a node is instantiated, I guess that makes sense at first, but might be reacting to "initialization" changes in an "unfinished" node since there might be be more init code inside those hooks.

Dunno, generalizing this is hard 💃

I'm starting to think it is just easier to have effect function that returns a disposer for early disposing and where the automatic disposing gets triggered depending on where you call the function

.actions((self) => {
  // created here (initialization phase) means auto dispose on beforeDestroy, ran before afterCreate/afterAttach
  effect(self, reaction(.....))

  return {
    afterCreate() {
      effect(self, reaction(...)) // will auto-dispose on beforeDestroy
    },
    afterAttach() {
      effect(self, reaction(....)) // will auto-dispose on beforeDetach
    },
    someOtherAction() {
      effect(self, ....) // or anywhere else: throw? allow and dispose on beforeDestroy to allow adding effects to a node when already created?
    }
  }
})

where effect can also be used for custom stuff

effect(self, () => { ... whatever returns a disposer })

and if the effect has to run only on certain conditions the user can just if/else based on getEnv, a property, a property of the root, process.env.NODE_ENV or whatever he wants

this would effectively be a "smarter" replacement of addDisposer

[k-g-a]

@xaviergonz I mean that if one describes effects on model type declaration, why would he want to turn those on or off?

const MyModel = types.model({props}).views(...).actions(...);
const MyModelWithEffects = MyModel.reaction(...);
/* or even */
const MyModel1 = types.model({props});
const MyModel2 = types.model({props});
const MyEffects = types.model().reaction(...);
const MyModelWithEffects1 = types.compose(MyModel1, MyEffects);
const MyModelWithEffects2 = types.compose(MyModel2, MyEffects);

/* and then instead of calling */
MyModelWithRunEffectsSetting.create({snapshot}, {runEffects: CONDITION})
/* one could just */
const Factory = CONDITION ? MyModelWithEffects1 : MyModel1;
Factory.create({snapshot});

Such approach makes code more self-documented as it implies concrete knowledge whether this particular instance contains effects or not. Meanwhile relying on ancestor's setting up the tree you will end up debugging your "supposed-to-run" effects in runtime - just to find out which of those ancestors accidantaly turned them off. Additionally, using composition will give you proper types for free: effects will be listed for models which have those and won't - for the rest.

For those who really want to have such a 'setting', it's not hard to implement (pseudocode):

const AutoEffectsModelBase = types
  .model()
  .volatile(self => ({
    __effectsEnabled: true
  }))
  .actions(self => ({
    afterCreate() {
      getMembers(self).effects.forEeach(key => {
        const originalEffect = self[key];
        self[key] = (data, reaction) => !self.__effectsEnabled && originalEffect(data, reaction);
      })
    },
    //automatic, can be enhanced in any way (i.e. through getEnv() or process.env.NODE_ENV)
    afterAttach() {self.__effectsEnabled = true},
    beforeDetach() {self.__effectsEnabled = false},
    //manual
    enableEffects() {self.__effectsEnabled = true}, 
    disableEffects() {self.__effectsEnabled = false}
  }))

@jayarjo , considering the branch you've mentioned: it narrows down every particular reaction to one field. So if one needs to react to subset of fields, he must first define a computed property (view), which just uses all the data needed, and then add a reaction to that view. This is actually not far from custom reaction at afterCreate.

[xaviergonz]

@k-g-a OK, maybe custom cases can be left for custom implementations, and certainly using models with and without effects can be controlled by using an extended model or not.

(still for these custom cases I think adding to add Disposer a parameter where you can configure on which phase they will be disposed would be cool 😎)

but on which lifecycle phase should they be started in your opinion?

'Additionally, using composition will give you proper types for free: effects will be listed for models which have those and won't - for the rest.'

why would effects need to be in the typings? they are kind of internal functions (unless you don't mean model instance typescript typings 😁)

[xaviergonz]

off topic : maybe there could be an afterInitialized hook with docs rather than expect people to know that stuff that runs on the top of views / actions /extends functions run on that phase?

[jayarjo]

So you guys are looking for something more exhaustive, than what this thread was originally about. I think that anything more complex than what I referenced in the branch doesn't worth the alternative implementation and will only confuse things.

afterCreate/afterAttach() {
      addDisposer(self, autorun/reaction(...));
}

Is already simple enough for all the cases beyond watching separate properties. IMHO.

[luisherranz]

I think it's a good opportunity to create an easier API for a common pattern :)

And maybe solve the lazy problem along with it.

[Amareis]

Main purpose of separated effects is improved readability and structure of code, I think.

Okay, maybe we went too deep for beginning of such complicated feature? Lets done something stupid and obvious and then improved that when users will complain about.

For example, only add chainable effects function, which is tiny wrapper on top of actions. User can calls effect (just as action) and it will automatically scheduled to dispose on destroy. Also on effect in runtime there is running property and stop() function, so user can manage them manually. He can run it on lifecycle hooks or from actions and stop if needed.
And, if there is effects on model, it's not lazy.

[xaviergonz]

something like:

.effects(self => ({
  whateverFx: reaction(() => self.foo, () => {...})
}), { autoStart: true/false} /* defaults to true*/)

and then access them through

getEffects(node).whateverFx.(running/stop()/start()) ?

??

About lazy, makes me wonder:
Since effects actually access props they want to react to and props accessed are auto-instantiated, does it really matter if they are lazy?

Also, again, what's a use case for manual stop, checking if it is running, etc? I'd love to see an example.

[Amareis]

No, even more stupid.
```js
.effects(self => ({
whateverFx: () => reaction(() => self.foo, () => {...})
})
.actions(self => ({
afterAttach() {
self.whateverFx() //run the effect, schedule it to be disposed on destroy
},
someAction() {
if (self.whateverFx.running)
self.whateverFx.stop() //dispose effect
//else effect will be disposed on destroy
}
}))

[Amareis]

In that case all is very explicit and user can do all sort of things that are possible with raw reactions, but effects are decoupled from actions and there is ways for next improving.

[xaviergonz]

fair enough by me :)
just a small change
self.whateverFx.start() //run the effect, schedule it to be disposed on destroy
since in the mobx world calling a function is usually akin to disposing

my only reservation is that I don't see it much of a change over self.addDisposer

[Amareis]

May be, not the point.

пт, 26 окт. 2018 г., 1:53 Javier Gonzalez [e-mail]:

fair enough by me :)
just a small change
self.whateverFx.start() //run the effect, schedule it to be disposed on
destroy

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/mobxjs/mobx-state-tree/issues/867#issuecomment-433201766,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AElX2XX1FfgGiXkynsAE8D1GVEjKGHtXks5uoiTWgaJpZM4UlHtu
.

[luisherranz]

 afterAttach() {
    self.whateverFx.start()
  },

The problem if we rely on afterAttach is that there's no way to be sure that an effect will run after model creation.

About lazy, makes me wonder:
Since effects actually access props they want to react to and props accessed are auto-instantiated, does it really matter if they are lazy?

That makes perfect sense, the effect could observe the props of the instance and therefore that instance would be created/attached. So no need to mark it as no-lazy by default.
– But this won't work if we rely on afterAttach as I explained above.

[Amareis]

This is why i wrote about disabling lazy mode if effects are declared on model. So, basically, effects doing 2 things - more decoupled code and solves lazy problem.

[luisherranz]

I'm not sure if disabling lazy mode when effects are present is the desired behaviour. Imagine this pattern:

const Parent = types.model('Parent', {
  children: types.array(Child),
  allChildrenActive: false
})

const Child = types.model('Child', {
  active: false
})
.effects(self => ({
  activateOnParentActivation: autorun(() => {
    if (getParent(self).allChildrenActive === true)
      self.active = true
  })
})

Sorry not to come up with a more realistic example, but if I'm not mistaken this effect would instantiate the parent but none of the children (as long as they are not observed) because the effect is observing the parent and not itself. So, it would still work fine, but it'd be more performant as the laziness would be still linked to runtime observation.

[Amareis]

I think that there is no place for laziness if any side effects in a game. But for similar cases there is can be some sort of immediate-on-instance-effects.

[xaviergonz]

since the effects would be manually started calling start(), the user can choose if they should start on initialization, afterAttach or wherever he wants.

I still don't see any issue with lazy in either approach.

• the user calls start() on the afterCreate phase, the reactions/autorun/whatever will touch the needed children and initialize them as well

• the user calls start() on the afterAttach phase, he probably has a good reason to do it there (most probably because the effect takes into account the parent) and again the reactions/autorun/whatever will touch the needed children/parents and initialize them as well. In this case the user will also call stop in beforeDetach (if he thinks that's the best place)

In either case the user can also select what's the best point for the child models to init their effects as well (be it afterCreate or afterAttach) and the best point to dispose them (be it beforeDestroy by default, or a custom call to stop on beforeDetach)

[xaviergonz]

or start() could even have an optional parameter to choose when the autodispose is done
start() - dispose on beforeDestroy
start('disposeOnDestroy') - same
start('disposeOnDetach') - dispose on beforeDetach

[xaviergonz]

or could even be inferred automatically from context
start() - dispose on beforeDetach if called from within afterAttach,, else dispose on beforeDestroy
start('disposeOnDestroy') - always on beforeDestroy
start('disposeOnDetach') - dispose on beforeDetach

[Amareis]

But effects can be with parameters?..

пт, 26 окт. 2018 г., 19:46 Javier Gonzalez [e-mail]:

or could even be inferred automatically from context
start() - dispose on beforeDetach if called from within afterDetach, else
dispose on beforeDestroy
start('disposeOnDestroy') - same
start('disposeOnDetach') - dispose on beforeDetach

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/mobxjs/mobx-state-tree/issues/867#issuecomment-433432855,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AElX2WRVnAvMyM40apFEQtNnqTjBX4s5ks5uoyAogaJpZM4UlHtu
.

[xaviergonz]

• run(...params) // choose from context the best place to dispose it (unless early disposed with stop)
• runUntilDestroyed(...params) // always dispose on beforeDestroy (unless early disposed with stop)
• runUntilDetached(...params) // always dispose on beforeDetach (unless early disposed with stop)
  then?

[xaviergonz]

However I have the hunch that what 90% of the people would really just need is

const NoOldValue= Symbol("mstNoOldValue")

types.model({x: 5})
.watch((self) => self.x, (self, newValue, oldValue /* NoOldValue on creation time */) => {
  ...
});

and they don't care about if it is attached, detached, or whatever :)

(and if they do they can just use isDetached, getParent, getRoot, getEnv etc over self)

[luisherranz]

I still don't see any issue with lazy in either approach.

• the user calls start() on the afterCreate phase, the reactions/autorun/whatever will touch the needed children and initialize them as well

• the user calls start() on the afterAttach phase, he probably has a good reason to do it there (most probably because the effect takes into account the parent) and again the reactions/autorun/whatever will touch the needed children/parents and initialize them as well. In this case the user will also call stop in beforeDetach (if he thinks that's the best place)

If the user has to start the effects on afterCreate or afterAttach, then we're back to the lazy issues/bugs.

Imagine my last example, with a small change: we want to sync with the backend

const Parent = types.model('Parent', {
  children: types.array(Child),
  syncWithBackend: false
})
.actions(self => ({
  setSyncWithBackend: val => { self.syncWithBackend = val }
}) 

const Child = types.model('Child', {
  id: types.identifier,
  name: types.string
})
.effects(self => ({
  syncWithBackend: autorun(() => {
    if (getParent(self).syncWithBackend === true)
      getEnv(self).api.syncChildren(getSnapshot(self))
  })
})
// let's start the effect in afterAttach, for example
.actions(self => ({
  afterAttach: () => {
    self.syncWithBackend.start()
  } 
})

If you set the parent syncWithBackend value to true:

const store = Parent.create({ children: [{ name: 'john' }, { name: 'peter' }] })
store.syncWithBackend(true)

Again, it's a silly example, but it illustrates the purpose.

This code is perfectly fine, easy to follow and well organized. But it won't work. The children won't send their info to the backend. They are not observed anywhere so afterAttach hasn't been called, thus effects are not started. This problem leads to difficult to find/predict bugs, with less than ideal fixes (place dummy observers in order to initialize children).

---

1. One solution would be to deactivate lazy mode for nodes containing effects. That would work, but I think it's inefficient. Here, you'd have to instantiate all the children. Imagine you have thousands of them :)

2. An alternative solution would be to run effects on store creation, without instantiating the object.

3. If the effect observers itself, then instantiation occurs <- this would be similar to solution 1.
4. But if the effect doesn't observe itself, you can bypass instantiation, saving some precious CPU cycles in the store creation.

Following my example with solution 2:

• Parent is created, but not instantiated.
• First child is created (john), but not instantiated.
• John's effect is run, observing Parent. Parent is instantiated.
• Second child is created (peter), but not instantiated.
• Peter's effect is run, observing Parent (already instantiated).

This way, we're back to the instantiated-when-observed formula of MST3.

[luisherranz]

By the way, this may seem like an edge case, but it did already happen in our app when using effects (autorun's inside afterAttach) and the code was so much simpler and clear that way that we finally decided to place dummy observers to solve it.

[xaviergonz]

Well, that's some weird example :)

so your main points (if I get it right) are that:
a) effects should always run, always on initialization (before afterCreate / lazy init), if they are defined (and depending on their internal code they decide if they should "run" or not)
b) there should therefore be no way to "start" them
c) there should be no way to "stop" them (is this one right?)
d) if done that way it should be ok to keep using lazy instantiation

[xaviergonz]

If so it sounds good to me as well, the only thing that worries me though is that at least "self" should be auto lazy initialized if effects are used (therefore the effects starting after self afterCreate), or else you could be using a self that is "incomplete"

[xaviergonz]

Basically:

init phase of a node that has effects (fxNode):

• fxNode gets inited, but not created (lazy inited)
• lazy-init nodes from ROOT to fxNode (this is to ensure the rule that afterCreate always triggers from root to children, and it is done already for example when accessing refs (https://github.com/mobxjs/mobx-state-tree/blob/4660d950169aefd91c1deb0a038afa19966464ab/packages/mobx-state-tree/src/core/node/identifier-cache.ts#L103))
• then register the effects

[luisherranz]

a) effects should always run, always on initialization (before afterCreate / lazy init), if they are defined (and depending on their internal code they decide if they should "run" or not)

That's right.

b) there should, therefore, be no way to "start" them

That's right.

c) there should be no way to "stop" them (is this one right?)

self.effectName.running and self.effectName.stop() are perfectly fine with this approach.

d) if done that way it should be ok to keep using lazy instantiation

I think so, yes.

If so it sounds good to me as well, the only thing that worries me though is that at least "self" should be auto lazy initialized if effects are used (therefore the effects starting after self afterCreate), or else you could be using a self that is "incomplete"

I'm not familiar with the lazy initialization internals. Maybe this behaviour is not possible after all.

What things can you access using self before initialization?

[xaviergonz]

What things can you access using self before initialization?

That's something @k-g-a can answer better than I can :) I'm curious as well of what would happen if a property is accessed outside any hook or action

But what I meant is something like this

props: x as number
afterCreate: self.x = self.x + 1 // for whatever reason
fx: based on self.x

given the object is created with x = 0
should the effect be added before afterCreate (so, the first reaction will be that x has become 1), or afterCreate (so it won't react)

also, what if you actually want to write an effect based on something that happens in afterCreate?

[xaviergonz]

Apparently bad stuff will happen

test("", () => {
    const M = types
        .model({
            x: 0
        })
        .actions(self => {
            expect(self.x).toBe(0)
            self.x = 5
            return {
                afterCreate() {
                    expect(self.x).toBe(5)

// this throws a weird exception if x is set outside afterCreate before
// something about the reconciliation algorithm not knowing what's going on
                    self.x = 6 
                }
            }
        })

    const m = M.create()
    expect(m.x).toBe(6)
})

so writing seems to kill it, reading seems fine though

[xaviergonz]

Funnily enough it doesn't happen with complex objects, only with primitives

test("", () => {
    const M2 = types.model({
        y: 0
    })
    const M = types
        .model({
            x: types.optional(M2, {})
        })
        .actions(self => {
            expect(self.x.y).toBe(0)
            self.x.y = 5
            return {
                afterCreate() {
                    expect(self.x.y).toBe(5)
                    self.x.y = 6
                }
            }
        })

    const m = M.create()
    expect(m.x.y).toBe(6)
})

[xaviergonz]

makes me think that stuff that happens inside initializers should be wrapped inside protect/unprotect

[Amareis]

You know, in Zen of Python there is wise rule: In the face of ambiguity, refuse the temptation to guess. And another one - explicit is better than implicit.

Complexity of automatic effects starting is problem which can be solved later. If we just disable laziness on models with effects and offers to users to start effects manually in lifecycle hooks, existing code will not slowed, and new code, using effects, should take into account possible performance issues. We even can marks effects as unstable feature and release it to collect feedback and real use cases. I think, it will much better than discuss in hundred comments.
Talk is cheap. Show me the code

[luisherranz]

My point is...

props: x as number
afterCreate: self.x = self.x + 1 // for whatever reason
fx: based on self.x

If the fx wants to access itself (i.e. self.x) then a property is observed and the node should be initialized before returning x. Therefore, if the fx access x there's no way it returns without executing afterCreate and afterAttach and that code would work as expected.

The cool thing is... if the fx doesn't want to access any property, there's no need to initialize the node.

[Amareis]

I think that is a good approach. But we need possibility to run effects manually, if there is stop function, why cannot be start?
For immediate effects we just need to add immediate option to effects, it will mirrors mobx reaction computed.observe and observable.observe behavior.

[luisherranz]

No problem with self.effectName.start/stop/restart(), whatever is useful. Also, no problem with the immediate or autostart to be opt-in as long as it runs on creation 👍

Buuut... I don't know if this suggestion is even possible. I hope @mweststrate, @mattiamanzati, @k-g-a or someone with more experience with the code can let us know.

[Amareis]

So, does we agreed to general effects API? In short, there is

.effects(self => ({
    effectName() {return disposer},  //effect returns itself disposer
    effectName2() {return [disposer1, disposer2]}  //or iterable of disposers
}), {startImmediately: true})  //false by default
.effects(self => ({
    effectName3(param, param2) {return disposer},  //effect can receive params
    effectName4() {return disposer},
}))
.actions(self => ({
    someAction() {
        self.effectName3.start(param, param2) //manually starts effect, schedule it to disposing on destroy
        if (self.effectName3.isRunning)  //check if running, value is observable to use it in reactions
            self.effectName3.stop()  //manually dispose effect and remove it from auto disposing
    }
}))

If startImmediately is true, effects starts right after node initializing, even if node is lazy.
All effects is disposed before destroing if not stopped before.

[Amareis]

And little enhancement - isRunning should be observed boolean. And instead of start/stop we can just assign true/false to isRunning, it maybe handsome in some effects running.
autorun(() => self.syncWithBackend.isRunning = getParent(self).syncWithBackend) will automatically manage effect. Of course it should not work with parametrized effects.

[luisherranz]

does we agreed to general effects API?

I love it!

[mweststrate]

Wasn't really following due to travelling, but looks neat :-). Are there some examples of what will some real(-ish) applications of effects will gonna look like with the new proposal?

[xaviergonz]

what if you set isrunning to true but the effect takes params to start? what if you set immediate to true but takes params? throw exceptions?

[Amareis]

Of course, if function length > 0 - fail.

[xaviergonz]

just one more minor thing, running rather than immediate?

[Amareis]

If you about naming, I think better using mobx terminology, where are exists immediate word.

[xaviergonz]

that's the point, fireImmediately and immediate sound close yet achieve different things

[xaviergonz]

the new one is like startImmediately

[Amareis]

Yeah, I think startImmediately is good enough.

[xaviergonz]

cool, edited the post to reflect that :) hope you don't mind

[Amareis]

Also edited to show isRunning using to manage effect.

[xaviergonz]

hmm, I'm not 100% sure I like setting the isRunning variable to actually do an action
you can do the same as this
autorun(() => self.syncWithBackend.isRunning = getParent(self).syncWithBackend)
with this

autorun(() => getParent(self).syncWithBackend ? self.syncWithBackend.start() : self.syncWithBackend.stop())

I think it is more clear and as long as multiple do-nothing calls to start() / stop() are allowed it should be ok (also it would not require throwing for parameter-ful effects)

[luisherranz]

I also think start() and stop() are more clear. effect.isRunning feels like a question instead of an action.

[xaviergonz]

which probably could just be named "running" but I'm ok with either :)

[k-g-a]

Do I understand it right that for user this would look like:

types.model({
    data: TheOtherModel,
    version: types.number
  })
  .effects(self => ({
    // this is called every time the version field is changed after this effect has been "started" by either "immediate" or manual ```.start()```
    synchronize() { MySynchronizer.sync(self.version) } 
  }))
  .actions(self => ({
    beforeDetach() {
      // this will turn off the effect, so for whatever reason version is changed on detached node
      self.synchronize.stop()
    },
    afterAttach() {
      // turn effect on after the node is reattached
      if (!self.synchronize.isRunning) {
        self.synchronize.start()        
      }
    }
  }))

Questions:
1) will effect body be automatically wrapped in autorun by MST (as above) or should user do it himself?
2) should self.synchronize.start() execute the effect or just mark it for execution on next observed data change?

I agree that isRunning should be readonly computed.
People usually want to use library/framework to write less code, plus declarative code is considered more readable, so I do think that startImmediately should default to true. Because if one declares an effect it should just work out of the box, without needing to know that you must start it somewhere or pass some magic option.
Same about effect receiving params through start() - this adds little convenience:

// you can write
someInstance.someEffect.start(foo, bar)
// instead of
someInstance.setFooAndBar(foo, bar)
someInstance.someEffect.start()

But one should guess that adding those parameters will force his effect to be manual.

I also think that enable()/disable()/isEnabled are more semantically correct. start()/stop()/isRunning sound like this effect is async and is being executed at the moment. I mean the body of the effect, like it was a flow() and it's in the middle of the yield.

[xaviergonz]

will effect body be automatically wrapped in autorun by MST (as above) or should user do it himself?

I think

  .effects(self => ({
    synchronize: () => autorun(() => { MySynchronizer.sync(self.version) }) 
  }))

should self.synchronize.start() execute the effect or just mark it for execution on next observed data change?

I think that's up to what you use (autorun/reaction/...) / the options that are passed (fireImmediately, etc), basicaly it is just executing the function and using self.addDisposer over its return data

People usually want to use library/framework to write less code, plus declarative code is considered more readable, so I do think that startImmediately should default to true. Because if one declares an effect it should just work out of the box, without needing to know that you must start it somewhere or pass some magic option.

Agreed

I also think that enable()/disable()/isEnabled are more semantically correct. start()/stop()/isRunning sound like this effect is async and is being executed at the moment. I mean the body of the effect, like it was a flow() and it's in the middle of the yield.

Agreed

Same about effect receiving params through start() - this adds little convenience:

Sorry, I'm not sure what you mean. so you think fxs with params add value or that they don't add value?

Now I guess the question is, when should startImmediately effects start, after afterCreate?

[k-g-a]

Sorry, I'm not sure what you mean. so you think fxs with params add value or that they don't add value?

I think we should not allow params for effects as this will impose the following rule: "if you add parameters to your effect it won't be started automatically (or will throw)" - it seems counterintuitive to change behaviour based on declared function arguments.
Moreover:

function myEffectHandler(...params) {
  console.log(...params)
}
console.log(myEffectHandler.length) // it's 0
myEffectHandler('foo', 'bar') // prints 'foo' 'bar'

I do not think we should teach peope that rest syntax is transpiled to arguments parsing internally ))

Now I guess the question is, when should startImmediately effects start, after afterCreate?

At quick glance it seems that finalizeCreation is a good place to start:

finalizeCreation() {
        // goal: afterCreate hooks runs depth-first. After attach runs parent first, so on afterAttach the parent has completed already
        if (this.state === NodeLifeCycle.CREATED) {
            if (this.parent) {
                if (this.parent.state !== NodeLifeCycle.FINALIZED) {
                    // parent not ready yet, postpone
                    return
                }
                this.fireHook("afterAttach")
            }
            this.state = NodeLifeCycle.FINALIZED
            for (let child of this.getChildren()) {
                if (child instanceof ObjectNode) child.finalizeCreation()
            }
           // everything is set up, can fire effects
        }
    }

But that's not for sure )

And once again, if we really want to bring ease of use, shouldn't we reduce boilerplate as much as possible?

types.model({
  data: TheOtherModel, // {name: 'Stuff', description: '...' price: 200, discount: 0.15}
  version: types.string
})
// i'd even call those 'autoruns' as it will become autorun internally
.effects(self => ({
    // this is called every time the version field is changed after this effect has been "started" by either "immediate" or manual ```.start()```
    synchronize() { MySynchronizer.sync(self.version) } 
  }))
.reactions(self => ({
  // this changes version every time data's name or description is changed, but ignores price/discount
  updateVersion: [
    () => ({name: self.name, description: self.descriptipn}),
    (changed) => {self.version = hash(changed.name, changed.description)},
    // those could be skipped completely, but give flexibility for ones in need
    {
      fireImmediately: false,
      equals: (prev, next) => {
        // consider editing as a change only if more than 2 chars of name or 5% of description have changed
        return getStringsDiff(prev.name, next.name).lenght> 2) || getStringsDiff(prev.description, next.description).percentage > 0.05)
      }
    }
  ]
})

But if we really can not agree upon fine grained API, I do not object implementing at least effects proposed in this comment.

[Amareis]

And once again, if we really want to bring ease of use, shouldn't we reduce boilerplate as much as possible?

Effects also can react to enviroment and changes node by actions. Reactions, autoruns etc should be just sugar on top of general effects.

I think we should not allow params for effects as this will impose the following rule: "if you add parameters to your effect it won't be started automatically (or will throw)" - it seems counterintuitive to change behaviour based on declared function arguments.

If you passes arguments to effect, it obviously cannot be run automatically, it's simple contract. We can check function length right on model creation, so user sees the error instantly. And you need this check even if forbid params totally, just it will be more strict.

Now I guess the question is, when should startImmediately effects start, after afterCreate?

No, it needs to run right in create function, because of lazy.

[Amareis]

Okay, let isRunning to be just a computed readonly boolean, comment updated again.

[luisherranz]

Is there any case where effects aren't going to include either autorun or reaction, or where you want to do some computation before starting them?

Because if that's not the case, I'd vote for the less-boilerplate version of .autoruns(self => ... and .reactions(self => ....

[Amareis]

In my current app there is a lot of api subscriptions and some dom event listeners, which also can be an effects.

[xaviergonz]

I'm also against boilerplate, but I gotta say that the more general version would always be up to date with whatever feature mobx throws next + custom ones without any need to implement it in MST...

Also implementing general effects + autoruns + reactions is not really exclusive, so they could be added later if needed (plus implementing autoruns/reactions on top of effects should be trivial)

[luisherranz]

Ok then 😊

[jayarjo]

Ehm... since this went nowhere. Lets maybe revive the original idea and this draft: https://github.com/mobxjs/mobx-state-tree/commit/1dfe7f4d691b2febe7d9c1fc5ff52157d22d39e2?

[cbovis]

We've been using volatile state to achieve this and it's been working well for us. Any downsides?

Our usage is structured like so:

// ...
.actions(self => {
  const root = getRoot(self);

  autorun(() => {
    self.someAction(root.otherStore.interestingValue);
  });

  return {
    someAction(val) {
      // do something with val to modify self
    }
  }
});
// ...

[mattruby]

I've done similar things, but I'll do them in afterAttach and then
cleanup in beforeDestroy https://mobx-state-tree.js.org/overview/hooks.
It's worked well for us.

On Tue, Jun 16, 2020 at 9:56 AM Craig Bovis [e-mail]
wrote:

We've been using volatile state to achieve this and it's been working well
for us. Any downsides?

Our usage is structured like so:

// ....actions(self => {
const root = getRoot(self);

autorun(() => {
self.someAction(root.otherStore.interestingValue);
});

return {
someAction(val) {
// do something with val to modify self
}
}});// ...

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/mobxjs/mobx-state-tree/issues/867#issuecomment-644818101,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AABCW4QYAF2GSPPKPGBN4HTRW6BZBANCNFSM4FEUPNXA
.

--
-Matt Ruby-
[email protected]