mobxjs / mobx-angular

The MobX connector for Angular.
MIT License
483 stars 59 forks source link

Automatic async actions #49

Closed robianmcd closed 4 years ago

robianmcd commented 6 years ago

@adamkleingit would you be interested in adding an action decorator that automatically wraps async operations in an action. I realized this could be done using zone.js to get a callback whenever something async happens.

I built a proof of concept for how this could work here: https://plnkr.co/edit/1S4UIt5LJ0VbVkYgsyei?p=preview

Basically you can annotate a function like this

@asyncAction
getDataWithHttpClient() {
  this.http.get('https://jsonplaceholder.typicode.com/posts/1')
    .map((data) => data.title)
    .subscribe((title) => this.httpClientTitle = title)
}

And the @asyncAction Will automatically wrap the callbacks in an action.

With mobxAngularDebug turned on my initial attempt was very verbose. Every .then() triggers a "microTask" in zone.js and was being wrapped in it's own action even if it wasn't doing anything async. I found that I could improve this by forking mobx, exporting startAction() and endAction() and using them to bundle together "microTasks" into a single action. This approach would need some work though as I think it currently breaks some of the spying functionality of mobx. @mweststrate what do you think about exposing some way of adding functions to a pending action?

Anyway let me know if you are interested. I'd be happy to clean this up and send a PR. Here's what I currently have for the implementation of @asyncAction

import {action, endAction, runInAction, startAction} from 'mobx';

declare const Zone;

const asyncAction = function (target, prop: string, descriptor) {
  let pendingAction;
  let finishedInitialSyncAction = false;

  //Create a zone that will wrap all async tasks in an action
  let actionZone = Zone.current.fork({
    name: `mobxActionZone:${prop}`,

    onInvokeTask(delegate, currentZone, targetZone, task, ...args) {
      if (task.type === 'macroTask' || task.type === 'eventTask') {
        return runInAction(`${prop}:async`, () => delegate.invokeTask(targetZone, task, ...args));
      } else {
        return delegate.invokeTask(targetZone, task, ...args);
      }
    },

    onHasTask(parentZoneDelegate, currentZone, targetZone, hasTaskState) {
      if (hasTaskState.change === 'microTask' && finishedInitialSyncAction) {
        if (hasTaskState.microTask) {
          pendingAction = startAction(`${prop}:async`, () => {}, null);
        } else if (pendingAction) {
          endAction(pendingAction);
          pendingAction = undefined;
        }
      }

    }
  });

  //Modify the annotated function so that it runs in the actionZone
  let origFunc = descriptor.value;
  descriptor.value = function (...args) {
    return actionZone.run(() => {
      finishedInitialSyncAction = false;
      let result = origFunc.call(this, ...args);
      finishedInitialSyncAction = true;
      return result;
    });
  };

  return action(target, prop, descriptor);
};

export default asyncAction;
adamkleingit commented 6 years ago

I guess it's not a bad idea, saves the trouble of breaking down the async action to multiple actions. But I don't think it should be part of MobxAngular - sounds like it should be its own library (because it might be relevant to React users as well). I wonder what @mweststrate has to comment on this.

mweststrate commented 6 years ago

@robianmcd @adamkleingit a generic solution is available in the mobxUtils package: asyncAcion, and based on generators. I think this zone based solution is a nicer alternative for Angular (less syntactic overhead).

So either putting it in mobx utils (under a different name, e.g. zoneAction) or in this package could both be a fine approach (as zone's are afaik only really used with angular although not limited to it?)

From technical perspective, beginAction and endAction should always happen within the same tick (so for every microtask you need to begin / end again). It is a bit unclear to me whether that is the case in your implementation or not. (As long as an action is running, mobx wouldn't be updating anything, so that means that the UI would freeze as long as the beginAction is not closed). That is why so far these methods are not exposed. But if this works accordingly they can be exposed through the extras namespace

robianmcd commented 6 years ago

@mweststrate Yeah this could go into the MobX utils.

My understanding of microtasks is that they will all run in the same tick and nothing can be scheduled in between them. So I don't think grouping them into an action could cause UI updates to freeze up. e.g. if I run:

console.log('start script');

setTimeout(() => console.log('setTimeout running'));

let promise = new Promise((resolve, reject) => {
  resolve();
});

promise
  .then(() => console.log('first then'))
  .then(() => console.log('second then then'));

console.log('end script');

First it will log

start script
end script

Then it will immediately run the micro tasks that were scheduled and log

first then
second then then

Then the browser's event loop will pop the next event off the queue and it will log.

setTimeout running

If the promise was actually doing something asynchronous like sending an http request then the callbacks would be scheduled as macrotasks instead of microtasks.

I came across a good article that explains this: https://jakearchibald.com/2015/tasks-microtasks-queues-and-schedules/

adamkleingit commented 6 years ago

@robianmcd, sounds awesome - can you make a PR? I want to make a blogpost about new improvements to mobx-angular (just added automatic detaching from the CD) - so it would be nice to include it (and credit you obviously)

robianmcd commented 6 years ago

Great I'll get to work on that. Which repo do you guys think makes the most sense? I guess the mobx repo would make it easier to tap into the internal startAction, endAction functions and maybe some react users would start including zone.js to take advantage of this. @mweststrate are you on board with this wrapping consecutive microtasks in a single action?