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?

[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 notifications@github.com:

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.

   • If the effect observers itself, then instantiation occurs <- this would be similar to solution 1.
   • 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 notifications@github.com 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- mattruby@gmail.com

[coolsoftwaretyler]

Hey folks,

I'm going through the issue tracker as I do from time to time, and this is currently the "Most commented" issue, but the discussion from years ago pretty much fizzled, and I'm in agreement with the prior two comments.

I write quite a lot of "effects" myself these days, and I almost exclusively use MobX utilities like reaction inside afterAttach and clean up with beforeDestroy (I store disposers as volatile state).

I think this is a great discussion overall, and my formal recommendation for anyone who wants this is to use that approach. It keeps the MST API the same (which is already somewhat bloated), and it helps to get you familiar with some of the underlying concepts from MobX that we're using.

I understand the desire to avoid "boilerplate", but I don't think this rises to the level that I'm concerned about.

I'm going to convert this issue to a discussion for posterity and easier discoverability (I find closed issues tend to be harder to find). Thanks for all your great ideas.

If anyone out there gets a notification for this, we're still around and doin' stuff! Come hang out and contribute!