Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fixed #750] Flow actions mutations not syncing to Redux DevTools. #1032

Closed
wants to merge 1 commit into from

Conversation

bourquep
Copy link

@bourquep bourquep commented Oct 4, 2018

By only hooking into the onAction notification, the Redux middleware was handling flow (async) actions at their beginning, where the state has not changed yet. So this MR injects another middleware to intercept flow_return action types, which is when the async action is done and the new state is available.

Ideally, we would exclude the regular action type for these, but at the time the middleware is invoked with call.type = "action" there is no way to determine if this is in fact an async action.

So, when an async action is invoked, the Redux DevTools will display:

myAction
myAction [flow_return]

And it's the [flow_return] one that will include the new state.

There weren't any existing tests for the Redux middleware, and I wouldn't really know how to implement them (looks like it would require mocking remotedev).

@xaviergonz
Copy link
Contributor

xaviergonz commented Oct 4, 2018

@bourquep makes me wonder, would it make sense for all the flow types to be shown on the redux dev tools?
the point being, what if a flow_resume of action A changes property (PA), then flow_resume of action B changes property PB, and then flow_return of B changes PB2, would the devtools assume PA, PB and PB2 changed because of action B because the resume ones were not tracked?

@xaviergonz
Copy link
Contributor

also while at it I'd refactor the "onAction" code to be inside the new middleware so there's less duplicated code

// an async action.
mst.addMiddleware(model, actionMiddleware)
function actionMiddleware(call: mst.IMiddlewareEvent, next: any) {
if (!applyingSnapshot && call.type === "flow_return") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably remove && call.type === "flow_return" and the onAction code

@bourquep
Copy link
Author

bourquep commented Oct 5, 2018

would it make sense for all the flow types to be shown on the redux dev tools?

This is what I did at first, but the result in devtools was a lot of "noise", at least in my use cases. For example, I have an async action that fetches updates from the backend via 3 different API calls and consolidates the result into a snapshot that is applied to my local MST tree.

In that case, I have 3 yields in my flow, but no state change in between them. The consolidated snapshot is only applied after the last API call.

So I ended up with useless resume actions logged to RDT, with no state delta.

But I agree that there are indeed uses cases where it would make sense to log all the flow types.

Ideally, the programmer should be able to annotate the flow to indicate which steps should be logged or not. But I am not sure how this could be done.

I'll experiment with ways to represent the various flow types in RDT, maybe there's a way to name the actions so the output is clearer (e.g. hierarchical by using whitespace or emojis).

@xaviergonz
Copy link
Contributor

xaviergonz commented Oct 5, 2018

or maybe it could detect if there was no change between the last snapshot and the new one and if that's the case then avoid showing the event (though that might be kind of slow for big states).

Maybe keeping a counter of json patch changes and when an action comes

  if (deltaPatches <= 0) skip logging
  else log and deltaPatches = 0;

?

This approach would also hide the actions at the start of a flow that actually do nothing.

And if this was somehow configurable even better (something like skipIdempotentEvents or whatever)

@bourquep
Copy link
Author

bourquep commented Oct 5, 2018

I have tried various things, but each attempt results in Chrome running out of memory. Not sure if it's an issue with devtools or with mst-middleware, but I don't have time right now to look into it.

@xaviergonz xaviergonz added never-stale Should never be marked as stale by the bot and removed never-stale Should never be marked as stale by the bot labels Oct 11, 2018
@bourquep
Copy link
Author

Closing this PR in favor of #1035

@bourquep bourquep closed this Oct 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants