Mobx-state-tree: RFC Introduce "action" in model.extend

[mattiamanzati]

@mweststrate if model.extend is not out yet, we should consider this :P

With the current API there's a subtle issue with calling actions not being wrapped by the result of .actions.
Take this example

const Todo = types.model({
   name: types.string,
   editedBy: types.string,
   done: types.boolean
}).actions(self => {
    function setEditedBy(user){
        self.editedBy = user
    }

    function toggle(){
        self.done = !self.done
        setEditedBy('N/D')
    }

    return {toggle, setEditedBy}
})

If you watch the log of the middleware when calling toggle, you'd expect both setEditedBy and toggle to appear in the log. Unfortunately the "setEditedBy" is not wrapped yet, so it won't.
A solution would be to call self.setEditedBy, but TypeScript would also complain about that because is not defined yet.

So what about introducing an "extend" that returns both functions and views and flow/processes?

const Todo = types
    .model({
        name: types.string,
        editedBy: types.string,
        done: types.boolean
    })
    .extend(self => {
        let sharedState = null

        const setEditedBy = action(user => {
            self.editedBy = user
        })

        const toggle = action(() => {
            self.done = !self.done
            setEditedBy("N/D")
        })

        const findById = flow(() => {
            yield http.get('....')
        })

        const viewFunction = (param: string) => {
            // stuff here
        }

        return {
            toggle, // action
            setEditedBy, // action
            get canToggle() { // computed
                return !!self.done
            },
            viewFunction // view,
            findById // process/flow
        }
    })

Basically action() will first check if running while a model is constructing, if not throw, and then wrap the function and return the already wrapped one. This way you can ensure that the resulting function will correctly emit middleware calls. The return value of the function given to .extend() will be simply Object.assign'ed to the just created instance. getter will be automatically converted into views. This also gives state that is shared across view and actions methods like #425 did.
Initially we could just map .actions and .views to a .extend, then maybe deprecate on future versions, like 1.5.0? This seems to solve some syntax issues.

Only "issue" in the implementation of this is how to get the function name while wrapping, ideally we could just lookup into "self" as those functions will be called only once the instance is created.

Please any comment from early adopter is welcome!

[mweststrate]

ideally we could just lookup into "self" as those functions will be called only once the instance is created.

Could you elaborate on that?

This might be a nice way to kill the other tree api's :) (existing extend, actions and views). Although all the action wrapping feels a bit verbose as well

[luisherranz]

I'm sorry to disagree but I think the props/views/actions API keeps things clearer and better organized.

I don't use typescript and not sure if this would work, but what about something like getEnv only for people using TS:

const Todo = types.model({
   name: types.string,
   editedBy: types.string,
   done: types.boolean
}).actions(self => {
    injectTS(self)

    function setEditedBy(user){
        self.editedBy = user
    }

    function toggle(){
        self.done = !self.done
        self.setEditedBy('N/D')
    }

    return {toggle, setEditedBy}
})

[mattiamanzati]

Yeah, my idea was to provide both methods, and don't deprecate the current one :)
Unfortunately something did like you said is impossible in TS, because you don't know yet the "end type" of "self" :/

[luisherranz]

Yeah, my idea was to provide both methods, and don't deprecate the current one :)

Oh, then I don't disagree anymore :)

[mattiamanzati]

Yeah, I agree they feel a little verbose, but if you think about it, they're just like plain mobx @action :)

So, here's some thought that will explain more to @mweststrate :)

With àction` we introduce a new modular way to extend model types, with some benefits:

Pros:

• function will be wrapped immediately, no more need of (self as any).method() to just get the method being emitted to middlewares
  Now this is tricky, because this case won't trigger any middleware:

const Todo = types.model({
   name: types.string,
   editedBy: types.string,
   done: types.boolean
}).actions(self => {
    function setEditedBy(user){
        self.editedBy = user
    }

    function toggle(){
        self.done = !self.done
        setEditedBy('N/D')
    }

    return {toggle, setEditedBy}
})

• we'll now get fully rid of "this", even if it were called self; calling self.method() is exactly the same as method()

Tricky parts:

• blocking non-exported middleware events:
  In the following example setEditedBy is wrapped by action, but not attached (a.k.a. returned) on the instance. When calling setEditedBy we could either throw or don't emit any middleware event.
  The latest one is kinda dirty, because you could setup a reaction with a action() and it won't be logged for time-travelling.
  It's safer to just throw saying "hey, you called this method, but you don't actually bound it to the instance!"

const Todo = types
    .model({
        name: types.string,
        editedBy: types.string,
        done: types.boolean
    })
    .extend(self => {
        let sharedState = null

        const setEditedBy = action(user => {
            self.editedBy = user
        })

        const toggle = action(() => {
            self.done = !self.done
            setEditedBy("N/D")
        })
        return {
            toggle
        }
    })

Same goes for calling action(fn) outside of a model constructor, it should throw.
How we can detect those cases and make it throw? The simplest approach that came to my mind is to just set a hidden property over the functions in the return of the () => {toggle}.
Inside action we can check if that property exists, and throw if not. That means that the function has been called before being attached to the instance.
Pseudocode:

// action()
function action(fn){
    let wrappedFn = function(){
        if(!wrappedFn.$attached){
            throw "You are calling a non-attached function!"
        }
        fn.call(null, arguments)
    }
    return wrappedFn
}

// ... on .extend(fn) implementation
const res = fn()
Object.keys(res)
    .filter(key => typeof res[key] === "function")
    .forEach(key => Object.defineProperty(res[key], "$attached", {value: true}))

Same goes for flow() implementation, which will basically return a action(fn), so that should work.

[ticruz38]

What about the sharedState variable, is it like a volatile state accessible from outside the model?

[mattiamanzati]

Yes indeed. It is a volatile state shared across all members inside the same "extend" call :)

[ticruz38]

Nice, will it be available in the next release? that would fit my need.
I understand that the volatile state is a 'bad practice' and most of the times we could get rid of that by implementing postprocessSnapshot on our model, but I still think it is more handy to have a place to define properties that want be snapshoted
The problem I have is when I need to observe some types on my model that aren't supported by MST, like React Element, or Moment, or an array of unknown element...
You want like it but that would be cool to have an 'any' type on our model

[mweststrate]

@ticruz38 note that non-serializable, observable properties can be created like this:

import { observable } from "mobx"

const X = types.model({}).extend(self => {
   const myElement = observable.ref(null)
   return {
       views: {
           get element() { return myElement.get() }
       },
       actions: {
          setElement(elem) { myElement.set(elem) }
       }
   }
}

It is a bit verbose, and we probably should introduce setters as well :) But alas these are all addressable problems. Feel free to open a new issue if we should

[mweststrate]

@mattiamanzati with the work in #332 we could actually detect / enforce that action is called inside actions and based on that automatically bind self correctly. But it is still a bit weird, as you will be creating an action that does trigger middleware correctly, but is not replayable as it is not invokeable from outside.

...Or we could add the action automatically to the instance, in which case we don't need the object returned from the initializer anymore :-D. Downside is that that would kill the typings for outside callers and the functions need to be named then.

[mattiamanzati]

@mweststrate one though I had was to have the action method behave like this

function action(fn: Function){
    let propName = null
    var newFn = function(){
        if(propName === null) throw "Action has not been bound!"
        return fn(...arguments)
    } 

    newFn.$wrap = (propertyName: string, self: any) => propName = propertyName
    return newFn
}

then when we attach the methods to the "self", we call fn.$wrap(propertyName) for each action so we ensure that the action is actually exported and the action name matches the exported one.

[mweststrate]

Eh and what problem did we then solve compared to just exporting the action the normal way? (I guess I should read the thread another time lol)

[mattiamanzati]

"but is not replayable as it is not invokeable from outside"
With that we can basically skip middleware emitting if the action is not returned in the object! a.k.a. invokable from outside :D

[mweststrate]

I think this one can be closed, correct @mattiamanzati ?

[mattiamanzati]

Yeah, will introduce something similar core-wise soon :)

[Ghirigoro]

Was something similar introduced?