xaviergonz / mobx-keystone

A MobX powered state management solution based on data trees with first class support for Typescript, support for snapshots, patches and much more
https://mobx-keystone.js.org
MIT License
546 stars 23 forks source link

Cancel flow actions? #261

Open evelant opened 3 years ago

evelant commented 3 years ago

In my (MST) app I have encountered the need to be able to cancel flow actions. Does mobx-keystone support this or could it easily support it?

There's a longstanding MST PR with discussion about it here https://github.com/mobxjs/mobx-state-tree/pull/691. I am currently running a fork of MST with flow cancellation and it works well.

Specifically the use case for this is long running flows that are yielding awaiting some condition that may or may not happen before the user navigates away or takes some other action that invalidates them. Without the ability to cancel flows these yields would never continue, resulting in a memory leak.

xaviergonz commented 3 years ago

You may use a cancellation token pattern to cancel flows/promises/anything async as shown here:

https://codesandbox.io/s/cancellation-token-kepth?file=/src/index.ts

evelant commented 3 years ago

@xaviergonz thanks for that example. I don't think that pattern would work for the case I'm envisioning. Imagine a flow action modeling a sequence of steps a user takes in a UI or a series of animations that need to play one after another:

runSequenceOfSteps =  _async(function* (this: MyModel) {
        for(const step of this.steps){
              //this promise might never resolve depending on what the user does
              //for example if they navigate away from the screen before the condition for the step is satisfied
             yield* _await(conditionsForStepPromise())
              //imagine these steps are complex, needing to call and await many other things
        }
}

//elsewhere in app
myModelInstance.runSequenceOfSteps()
//now it is possible for runSequenceOfSteps to yield forever because a promise for a step
//might never get resolved or rejected!

It would be beneficial to be able to cancel a flow like this that could yield a promise that might not ever get resolved. Especially in more complex user interaction cases like a series of steps outlined above. I think it's probably fairly common at least in UI for async actions to become "invalid" or no longer relevant while they await some condition.

The only alternative I can see to being able to cancel flows is to painstakingly ensure that every promise yielded in a flow will resolve/reject in every possible case in which the flow might need to cancel. By doing that I think it would be possible to use the cancellation token pattern you suggested. I think however it would be quite easy to miss a case and end up with a flow "stuck" on a promise that will never resolve and cause a memory leak or unexpected behavior.

I know promise cancellation is not a standard thing, but so far I can't think of another way to handle this sort of situation gracefully.

This is how I am using a patched version of MST with flow cancellation:

    .actions((self) => {
        let pendingPromises: CancellablePromise<FlowReturn<any>>[] = []

        /**
         * Based on https://github.com/mobxjs/mobx-state-tree/issues/681
         * Added a .cancel() function to flows so they can be stopped if they're waiting for a yield
         *
         * This wraps a flow and keeps a reference to its promise so it can be cancelled
         * @param generator
         */
        function trackedFlow<R, Args extends any[]>(
            generator: (...args: Args) => Generator<Promise<any>, R, any>
        ): (...args: Args) => Promise<FlowReturn<R>> {
            //Make flow function
            const theFlow = flow(generator)
            //Return a wrapper function that keeps track of when it's called and cleans up after it
            return function wrappedFlow(...args: Args): Promise<FlowReturn<R>> {
                //Call the flow
                const p = theFlow(...args)
                //Record the promise
                pendingPromises.push(p)
                //When promise is resolved or rejected remove the flow from tracked
                p.finally(function removeCompletedFlow() {
                    _.remove(pendingPromises, (pending) => pending === p)
                })
                //Return the promise to caller
                return p
            }
        }

      const myAsyncAction = trackedFlow(function* myAsyncAction( ) {
            .....
            yield when(() => self.somethingThatMightOrMightNotHappenBeforeDestroy)
            .....
      })

     function beforeDestroy() {
            //Cancel all outstanding flows when the model is torn down
            pendingPromises.forEach((p) => p.cancel())
            pendingPromises = []
        }

    return {
        beforeDestroy,
       myAsyncAction
     }
})

Essentially there are conditions in which running flows no longer matter to the app. They just need to be stopped and cleaned up right away regardless if they're awaiting a promise or not. A cancel method for flows makes that case easy and clean to handle.

I could also be missing a better way to handle this case however!

xaviergonz commented 3 years ago

What about using a lib that supports cancellable promises (e.g. bluebird http://bluebirdjs.com/docs/api/cancellation.html) and do state mutation via sync actions inside that promise?

You might even group all the state mutations inside a single undo step using createGroup from undoManager if that's an issue (or even better, do all the state mutations at the end of the promise if that's a possibility).

evelant commented 3 years ago

I think that would probably work but might have a few downsides, primarily

  1. Requires another lib
  2. Requires writing cancellable flows in a "special" way different from the standard mobx-keystone flows
  3. Requires defining sync actions for all mutations even if the only place they're used is in the cancellable flow

I'll have to give this approach a try when I get some time to see how it pans out in practice however.

yuqianma commented 2 years ago

Mox's flow is "cancellable". Can mobx-keystone follow this way? https://mobx.js.org/actions.html#cancelling-flows-

calclavia commented 2 years ago

Would be nice to support this without using cancellation tokens