mobxjs/mobx-state-tree

production build seems to shrink asynchronous actions name (middleware)

Closed this issue · 23 comments

fabienjuif commented

image

version: 1.1.0 / 1.0.0

I have an action named login, at start I can catch it (type action) :

{
  fullname: "/auth/login",
  name:"login",
  path:"/auth",
}

At the end I can't catch it (type flow_return) :

{
  fullpath: "/auth/t",
  name: "t",
  path: "/auth",
}

I saw this bug using trampss-mst-onaction.
But I wanted to try with a simpler middleware to see if the error comes from me :

  addMiddleware(store, (call, next) => {
    const { type, name } = call
    console.log('bug ?', { type, name })

    next(call)
  })

And it seems that this is the build version of mobx-state-tree :(

image

mweststrate commented
fabienjuif commented 
export default types
  .model({
    user: types.maybe(User),
  })
  .actions(self => ({
    setUser: (user) => {
      localStorage.setItem('user', JSON.stringify(user))

      self.user = user
    },
  }))
  .actions(self => ({
    init: () => {
      const raw = localStorage.getItem('user')
      if (raw) self.setUser(JSON.parse(raw))
    },
    login: process(function* login(auth) {
      // query
      const query = gql`
        query {
          user (email: "${auth.email}", password: "${auth.password}") {
            id
            token
          }
        }
      `
      const { data: { user } } = yield client.query({ query, fetchPolicy: 'network-only' })

      // update store and localstorage
      if (user) self.setUser(user)
    }),
    logout: () => {
      localStorage.removeItem('user')
      self.user = null
    },
  }))
  .views(self => ({
    get token() { return self.user && self.user.token },
    get logged() { return !!self.token },
  }))

I thank about the uglifier, but then how the type action retrieve the right name ?

fabienjuif commented

Ok https://github.com/mobxjs/mobx-state-tree/blob/master/packages/mobx-state-tree/src/core/flow.ts#L43

The name for asynchronous actions is not the field name declared into mobx-state-tree types.actions, but this is the function name provided to flow decorator:

    login: flow(function* /* this is it -> */ login(auth) { }

So this is why uglifier shrink it :(

fabienjuif commented

@mweststrate @mattiamanzati

Wild idea, what if we declared asynchronous action this way ?

const Store = types
  .model({})
  .actions(self => ({
    login: function* login() { /* ... */ },
  })

And then types.actions decorate himself the action if it detects this is a generator ? (at instanciation). So it can catch the good name !

Then into flow implementation we can add a parameter, the name (which is the field from the object returned by the types.actions callback) :

export function flow(name: string, asyncAction: any): any {
    return createFlowSpawner(name, asyncAction)
}

Advantages :

  • We don't import an other API as mobx-state-tree user (flow is not needed to be public anymore)
  • We declare actions almost the same way (synchronous and asynchronous ones)
  • Uglifier shrink is avoided

Drawbacks:

  • flow API should be changed
  • this is a breaking change (?)

edit:
TL&DR: move the flow decorator responsability from developer to mobx-state-tree, and then it will patch the uglifier bug.

edit2: warning added to trampss-mst-onaction

  • 👍1
fabienjuif commented

Something like this: https://github.com/fabienjuif/mobx-state-tree/pull/1/files

I have to test it, I am really not sure about this.

mattiamanzati commented

We had something like that, the problem is that unfortunatelly if generator gets transpiled the check we used wont work :/
Please have a look at #456 , we could attach to the returned function a hidden method that will be called when the function will be attached to the MST instance.
So we will know the exact name used in the return of the .actions()

fabienjuif commented

That's just what i try here @mattiamanzati I think : https://github.com/fabienjuif/mobx-state-tree/pull/2/files

edit: as I come to the transpiled conclusion :( https://github.com/fabienjuif/mobx-state-tree/pull/1/files#r148020321

edit2:

  • Add a field $mst_flow into flow decorator
  • createFlowSpawner returns a new function which take the name
  • and then in model.ts :
if ((action as any).$mst_flow) {
  action = (action as any).spawner(name)
}
mweststrate commented
  • 👍1
fabienjuif commented

@mweststrate I prefer not to specify name 3 times (action field, into flow parameter, and as a named function)

I think with this start of solution it could work: https://github.com/fabienjuif/mobx-state-tree/pull/2/files

I will read #456 as @mattiamanzati suggests

mattiamanzati commented

@fabienjuif I'll write some pseudocode tomorrow, or maybe even an implementation :)

mweststrate commented
mweststrate commented
fabienjuif commented

@mattiamanzati It will be great if you look at my second PR linked above and tell me what's wrong with this idea (so I can understand 😊)

mattiamanzati commented

https://github.com/fabienjuif/mobx-state-tree/pull/2/files#diff-060ef979aa0afd5ee7c8fe9cbd2b69edR43
The problem is that you are returning a object instead of a function, so this would break :)

.actions(self => {
   const internalSave= flow(function*(){

   })

  return ({
    save: flow(function*(){
       // do stuff...
       yield internalSave()
    })
  })
}
mattiamanzati commented

@fabienjuif #456 (comment) replace "action" with flow, and you'll basically get the gist :)

fabienjuif commented

Ok I understand it now :) Thank you !

mattiamanzati commented

Ideally I would love a "createDecorator(fn)" api that returns a new function, and copy the metadata from the previous fn (like we do with $mst_middlewares)

  • 👍1
fabienjuif commented

We could decorate the function returned by createDecorator(fn), and inject the action name via model.ts like I did with my second PR.

But your snippet still not works with this idea:

.actions(self => {
   const internalSave= flow(function*(){

   })

  return ({
    save: flow(function*(){
       // do stuff...
       yield internalSave()
    })
  })
}

How do internalState knows the action name?
What is the action name here ? internalState or save?

I'm getting confused.

mattiamanzati commented

In that case internalSave wont emit any middleware event, as it is not an exported function :)

  • 👍1
hmedney commented

Interested in this one also - in preserving async action names in uglified builds. Would it be an appropriate use of MST to implement logic that depends on whether certain actions have been invoked, just by capturing action names via addMiddleware()? Any risks/downsides to this approach?

For example, knowing whether a particular async action had ever been invoked successfully (or is currently running) can enable the presentation layer to either show a loading message or display any existing data already in the store.

I think my net question is whether preserving the names of async actions is a design goal for MST and whether building logic based on call objects received through addMiddleware() is sound practice.

Thanks, and thanks for creating this awesome library!

fabienjuif commented

@hmedney this is all about the lib I created here and this is why I dig this issue :)

https://github.com/alakarteio/k-mst-onaction

hmedney commented

Hi @fabienjuif ah, nice - like the global function approach. I was going down a different path of capturing all the async actions in a MST tree and then observing changes in the tree.

@mweststrate @mattiamanzati It's somewhat implicit in this issue, but I was wondering if you could confirm before I go too much further down this road - is basing logic on async action events via addMiddleware() a sound practice in MST? Whether certain async actions have run, have never run, are running, or have error'd can be useful for the view to show a busy indicator, existing state, or an error message in a component rendering state from an async source.

mweststrate commented

Closing this issue as it is inactive for quite a while. Best open new issues for new questions.