fix nested flows with middleware

[davidyxu]

For the issue mentioned in https://github.com/mobxjs/mobx-state-tree/issues/1271

[davidyxu]

@xaviergonz

I started looking getting the onResume and onSuspend hooks in on this fork: https://github.com/davidyxu/mobx-state-tree/commit/cd1b71c2d9308ac37c1bf6c7f20adad5678b1058

But I'm still seeing a similar issue as with the old API, where the outerFlow hooks aren't being called again when the innerFlow resumes (which I would expect to happen because in this case we're calling innerFlow within the context of the outerFlow, so the middleware on innerFlow and outerFlow should both be used firing).

    outerFlow [ 'outerFlow (27) - onStart',                                                                                                                  
      'outerFlow (27) - onResume',                                                                                                                           
      'outerFlow - run 1',                                                                                                                                   
      'innerFlow (29) - onStart',                                                                                                                            
      'innerFlow (29) - onResume',
      'innerFlow - run 1',
      'innerFlow (29) - onSuspend', 
      'outerFlow (27) - onSuspend',
// would expect outerFlow - onResume here, but there is not
      'innerFlow (29) - onResume',
      'innerFlow - run 2',
      'innerFlow (29) - onSuspend',
      'innerFlow (29) - onFinish (error: false)',
// would expect outerFlow - onSuspend here, but there is not (or maybe before the onFinish, unsure of expected behavior here)
      'outerFlow (27) - onResume',
      'outerFlow - run 2',
      'outerFlow (27) - onSuspend',
      'outerFlow (27) - onFinish (error: false)' ]

For this particular case I could fix it by basically iterating over the call chain and digging up runningActions for parents and calling their onResume/onSuspend in the right order.

However, I don't think this approach would work in more complicated cases because to my understanding, the weak map would only contain running actions for the same middleware. If for example outerFlow was decorated with a different middleware (but innerFlow wasn't), then there isn't a way with the data model to correctly fire off the outerFlow's onResume hook.

It's a pretty significant change, but I think a possible solution would be to have a global map of calls to an array of hooks that should be called.

[xaviergonz]

Can you share your unit test code? I might give it a try

[davidyxu]

It should be in the link that I shared: https://github.com/davidyxu/mobx-state-tree/commit/cd1b71c2d9308ac37c1bf6c7f20adad5678b1058#diff-c64a56beaa47a06dd719c54b249a5823

Currently it's just doing console.logs instead of actual expectations but should be pretty straight forward to fix that

And in case it was unclear, sync is testing the synchronous middleware flow, innerFlow is testing a single flow middleware, outerFlow is testing nested flow middlewares.

I haven't written any tests to explore different middlewares on diifferent actions via decorate yet, that would be the next step once i get outerFlow working. Then aside from that, making sure all of those combinations interacts correctly with the filter function.

[davidyxu]

@xaviergonz Just wanted to check in to see if you had any luck on your end investigating this. Happy to help setup a few more unit tests or try out any alternative approaches.

[mweststrate]

@xaviergonz @davidyxu I notice that this one is open for a really long time already :). Any suggestions on how to proceed?

[davidyxu]

@mweststrate This PR will fix the exception in the linked issue using the original middleware API. I also have a branch which extends the middleware 2 API with onResume/onSuspend flags.

Unfortunately the way in which middlewares fire when combined with flows is a little unexpected, and from my limited understanding of the MST codebase, it would require some refactoring in the middleware system to get into a more .

If other people are running into the same issue with flows and middlewares mentioned below, I think it makes sense to merge this PR: https://github.com/mobxjs/mobx-state-tree/issues/1271

The issues around when middleware hooks are called during flows could be tracked with a new giithub issue.

[mweststrate]

@davidyxu so if I understand correctly from the issue, you were able to fix the problem with then existing middleware api's, by doing some more work, correct?

[xaviergonz]

I tried to make it work without tweaking many things but failed.

To make this properly work we'd need a refactor to the action context internals so that action each contexts include their root context, the async action that spawned the action context and so on. I did it for mobx-keystone, so the idea is sound, but it is no small feat.

Relevant files if interested: https://github.com/xaviergonz/mobx-keystone/blob/master/packages/lib/src/action/context.ts https://github.com/xaviergonz/mobx-keystone/blob/master/packages/lib/src/action/modelFlow.ts https://github.com/xaviergonz/mobx-keystone/blob/master/packages/lib/src/action/wrapInAction.ts https://github.com/xaviergonz/mobx-keystone/blob/master/packages/lib/src/actionMiddlewares/actionTrackingMiddleware.ts

[davidyxu]

@mweststrate yes, the immediate problem of the exception is fixed in this PR, but @xaviergonz's suggestion of checking the order in which middleware hooks are called revealed that there's deeper issues to address which requires a larger refactor.

[jamonholmgren]

Hey folks, I'm taking over MST as primary maintainer. Could I get an update on where this PR is at and whether you think it'll move forward or if we should close it for now? 🤗

[jamonholmgren]

Gentle ping @davidyxu ^^

[davidyxu]

I haven't touched MST in quite a while, but as far as I remember, the PR does fix the uncaught errors mentioned in the linked issue. It however does not fix the underlying issues around the ordering of multiple middlewares.

Those underlying issues look fixable but would involve a much larger refactor than I'm comfortable committing to.

TLDR: This PR is a small improvement to the original issue, but a larger change is necessary to fix everything discussed here. Please feel free to close it if you'd prefer to wait for a more comprehensive fix.

[jesse-savary]

@jamonholmgren I feel that fixing a bug is worth it even though certain MST internals do need to be reworked to solve greater issues. Thoughts?

[jamonholmgren]

@jamonholmgren I feel that fixing a bug is worth it even though certain MST internals do need to be reworked to solve greater issues. Thoughts?

@jesse-savary I'd be willing to review a new PR that fixes those greater issues; I won't be pushing this forward myself, though, as it's still a fairly unusual edge case to run into any of these. Are you looking for something to work on? 😅