store icon indicating copy to clipboard operation
store copied to clipboard

🐞[BUG]: NgxsReduxDevtoolsPluginModule logs actions in wrong order when dispatching actions from async actions

Open perjansson opened this issue 6 years ago β€’ 39 comments

Versions

* ngxs: 2.0.0-rc.18
* @angular/core: 5.2.1

Repro steps

When dispatching RequestSearchOptions still RequestSearchOptionsSuccess logs before RequestSearchOptions in ReduxDevTools.

@Action(RequestSearchOptions)
requestSearchOptions({ getState, dispatch }: StateContext<SearchStateModel>) {
	const fetchSearchOptions = async () => {
		const searchOptionsResponse = await provider
			.getOptions()
			.first()
			.toPromise()

		dispatch(new RequestSearchOptionsSuccess(searchOptionsResponse))
	}

	fetchSearchOptions()
}

Observed behavior

NgxsReduxDevtoolsPluginModule logs actions in wrong order when dispatching actions from async actions.

perjansson avatar Mar 22 '18 20:03 perjansson

I am seeing that too. I have a different pattern for my calls:

 @Action(LoadAllQuotes)
  async loadAllQuotesAction({ getState, setState, patchState, dispatch }: StateContext<IQuoteStateModel>) {
    patchState({ quotes: [], selectedQuote: null, busy: true });
    const data = await this.service.getQuotes().toPromise();
    return dispatch(new LoadQuotesSuccess(data));
  }

  @Action(LoadQuotesSuccess)
  async loadQuotesSuccessAction(
    { getState, setState, patchState, dispatch }: StateContext<IQuoteStateModel>,
    { payload }: LoadQuotesSuccess
  ) {
    patchState({ quotes: payload, busy: false });
  }

I was using observables, but when I upgraded from ngsx 1.x to 2.0 RC20, the observables stopped working. I debugged and saw that the underlying line that calls HttpClient get was being hit, but the network call was never attempted. After hours, I tried the async/await pattern and the app works correctly (and I really like how the code looks now!), but paired actions in the store show up reversed in the Dev Tools.

RobCannon avatar Mar 24 '18 03:03 RobCannon

If you wrap dispatch() with setTimeout() it'll be in the correct order. But that shouldn't need to be required.

trevor-hackett avatar Mar 24 '18 03:03 trevor-hackett

Yes, I just confirmed that. I think the entries into DevTools are being completed when the @Action function completes and not when patchState or setState are called.

The sequence seems to be this:

  1. First Action starts
  2. First Action modifies state
  3. First Action dispatches Second Action
  4. Second Action modifies state
  5. Second Action function completes and returns to First Action
  6. Second Action added to DevTools
  7. First Action function completes
  8. First Action added to DevTools

RobCannon avatar Mar 24 '18 14:03 RobCannon

@RobCannon can you try with the latest version and confirm if you are still seeing the issue?

deebloo avatar Mar 27 '18 18:03 deebloo

Reproduce: https://stackblitz.com/edit/ngxs-simple-luec15

trevor-hackett avatar Mar 27 '18 20:03 trevor-hackett

Ok, reading through this and I can confirm this is working as designed right now (now im not saying thats right but its how it is).

So heres the gist:

  • Plugins run in a middleware chain, plugin -> plugin -> action
  • All actions return an observable (whether you do or not)
  • When a action's observable completes, the devtools plugin listens and sends the action
  • The actions operate autonomous of other actions, so if i were to fire 3 actions and the 2nd one had a ajax call in it, 1 and 3 would come back first since they completed.

We need to wait for completion before we send the action to the devtools. We also don't want to block any other actions from dispatching since one could be long running. It would also be incorrect to show them in sequential order since they actually didn't run that way. They might have been dispatched that way but not actually executed in that flow.

amcdnl avatar Mar 27 '18 21:03 amcdnl

@deebloo I am still seeing states in the "reverse" order after upgrading to @ngxs/[email protected].

I think the issue is with the semantics of dispatch(). It looks like dispatch immediately executes the supplied Action. I would expect that dispatch would add the Action to a queue and once the current Action completes, ngxs would see if there is anything left in the queue.

I tried to look at the source code to see if that is true, but I got lost trying to track the Observables. I can use Observables, but I don't totally grok them enough where I can figure out what is going on in someone else code.

I was having troubles with Observable based actions, but now that the code is out of RC, I will give that a try again and see if that changes anything.

RobCannon avatar Mar 30 '18 23:03 RobCannon

I tried Observables again and that is working now, but the DevTool is still showing actions in reverse order.

Here are the two version of code that I am trying:

Observable version:

  @Action(LoadAllQuotes)
  loadAllQuotesAction({ getState, setState, patchState, dispatch }: StateContext<IQuoteStateModel>) {
    patchState({ quotes: [], selectedQuote: null, busy: true });
    return this.service.getQuotes().pipe(map(data => dispatch(new LoadQuotesSuccess(data))));
  }

  @Action(LoadQuotesSuccess)
  loadQuotesSuccessAction(
    { getState, setState, patchState, dispatch }: StateContext<IQuoteStateModel>,
    { payload }: LoadQuotesSuccess
  ) {
    patchState({ quotes: payload, busy: false });
  }

Async version:

  @Action(LoadAllQuotes)
  async loadAllQuotesAction({ getState, setState, patchState, dispatch }: StateContext<IQuoteStateModel>) {
    patchState({ quotes: [], selectedQuote: null, busy: true });
    const data = await this.service.getQuotes().toPromise();
    return dispatch(new LoadQuotesSuccess(data));
  }

  @Action(LoadQuotesSuccess)
  loadQuotesSuccessAction(
    { getState, setState, patchState, dispatch }: StateContext<IQuoteStateModel>,
    { payload }: LoadQuotesSuccess
  ) {
    patchState({ quotes: payload, busy: false });
  }

RobCannon avatar Mar 31 '18 00:03 RobCannon

This is similar to #213

amcdnl avatar Apr 12 '18 13:04 amcdnl

Should be fixed in 3.0

amcdnl avatar May 08 '18 13:05 amcdnl

Hi, I have the version 3.0.1 and also I have the problem. Have You fix?

foralobo avatar May 20 '18 11:05 foralobo

@markwhitfeld - shouldn't the ordered actions fixed this?

amcdnl avatar May 20 '18 15:05 amcdnl

@amcdnl In theory it could have fixed it but I think we made an assumption here about the cause. I can confirm that the issue is still happening with 3.0.1. I forked the repo above and updated the deps (love the update all deps to latest button in stackblitz! ): https://stackblitz.com/edit/ngxs-simple-issue-139-repo?file=app/app.state.ts I think that we should reopen this issue.

markwhitfeld avatar May 21 '18 13:05 markwhitfeld

I had a look at the code here and it sends the info to the redux dev tools when the action has completed. This makes sense because the dev tools requires the new state as part of an action's payload.

By design in NGXS the parent action only completes when the child action completes, so because the child action is completing before the parent it is posting its completed state to the dev tools before the parent.

@amcdnl We will need to figure out how to handle this correctly... There are probably a few ways we could go here. But just a quick thought is that we could reflect an action in multiple states because we have more defined lifecycle stages for actions than redux:

  • On Dispatch
  • Every time it triggers a state change
  • On Completion/Error/Cancel

markwhitfeld avatar May 21 '18 14:05 markwhitfeld

considering this is clearly still happening - can we reopen the issue?

Doug-Shannon avatar Jun 07 '18 03:06 Doug-Shannon

Re-opening because we need to figure out an approach that makes sense for the redux dev tools. PS. We are in the process of making our own dev tools that will fit the NGXS paradigms better. (See https://github.com/ngxs/devtools )

markwhitfeld avatar Jun 20 '18 20:06 markwhitfeld

Thanks for reopening the issue. While this is not a crucial topic, it is kind of irritating to have the wrong order. This misses an important point about the Action -> ActionSuccess || ActionError pattern, which most of us love using.

May I ask what are your motives behind the decision to create a new devtools plugin? I myself would feel more comfortable to use the same plugin for all state instances I have open, whether ngrx, NGXS or something else. I find Redux DevTools pretty good and I don’t really see a significant ROI of a new plugin (please enlighten me!). Has anyone investigated whether replicating ngrx’s way is not going to fix this issue?

P.S. Please also update the issue tags.

filipjnc avatar Jun 20 '18 22:06 filipjnc

@filipjnc There is much richer information that we are able to gather about the actions that we would want to display in the dev tools. Something that the standard redux dev tools won't support. When you see it you will know why ;-) It will be a while before this is ready though, so we do need to look at better options of displaying info in the redux dev tools.

Ngxs's approach allows for a bit more flexibility than the typical redux style frameworks and as a result some of the paradigms don't fit nicely into what the redux dev tools expect. Actions dispatched as a part of a parent action are considered to be children and as a result the parent action only completes once the children complete. The completion states are what you are seeing in the dev tools, hence the out of order listing.

markwhitfeld avatar Jun 21 '18 08:06 markwhitfeld

@markwhitfeld Now I see, thanks for the explanation! :)

Regarding Redux DevTools, I'd go with the option to log at dispatch (at least for the actions with children). This implies that cancelled or failed actions will also get logged, though I think this is also how ngrx works as well. A potential workaround would be, provided Redux DevTools support, to remove the action log once failed or cancelled, or to add the failure/cancellation as a separate log (this can be confusing, but less confusing and would occur way less often than the issue we try to solve). Both ways would follow the principle of an optimistic logging. What do you think?

filipjnc avatar Jun 21 '18 09:06 filipjnc

Logging the dispatch only will also give an unexpected result because the state is not changed at dispatch. This is why I thought about logging everything with their lifecycle stage included. See: https://github.com/ngxs/store/issues/139#issuecomment-390673126

markwhitfeld avatar Jun 21 '18 12:06 markwhitfeld

Any news on this Topic? I want to use ngxs instead of ngrx but this is a major drawback

headsetsniper avatar Aug 19 '18 11:08 headsetsniper

Really hard to track app state when this is happening ;/ Any new on this?

Karolis92 avatar Dec 04 '18 14:12 Karolis92

Another issue with the DevTool showing actions in reverse order, is that the state changes are not displayed properly on the parent Action. If the parent action changes a property and the same property is then changed on the child, the DevTool shows no change on the parent.

In this example if the service errors out the DevTools shows: LoadQuotesFail LoadAllQuotes And both have no state changes because the busy property changes to true and then back to false.

@Action(LoadAllQuotes)
loadAllQuotesAction({ getState, setState, patchState, dispatch }: StateContext<IQuoteStateModel>) {
  patchState({ busy: true });
  return this.service.getQuotes().pipe(
              map(data => dispatch(new LoadQuotesSuccess(data))),
              catchError(error => of(dispatch(new LoadQuotesFail(error))))
              );
}

@Action(LoadQuotesSuccess)
loadQuotesSuccessAction(
  { getState, setState, patchState, dispatch }: StateContext<IQuoteStateModel>,
  { payload }: LoadQuotesSuccess
) {
  patchState({ quotes: payload, busy: false });
}

@Action(LoadQuotesFail)
loadQuotesFailAction(
  { getState, setState, patchState, dispatch }: StateContext<IQuoteStateModel>,
  { payload }: LoadQuotesFail
) {
  patchState({ busy: false });
}

lalmanzar avatar Feb 04 '19 16:02 lalmanzar

This issue was solved? I am still getting this behavior.

guilhermegjr avatar May 15 '19 21:05 guilhermegjr

@guilhermechiara sorry, but the problem is still not solved

splincode avatar May 15 '19 22:05 splincode

The devtools of ngxs seem to be abandoned. At least from what I could gather from https://github.com/ngxs/devtools/issues/118 and the repo.

So it comes down to fixing it in the plugin I guess? Are people using other ways to visualize their state, that I missed?

razzeee avatar Jan 24 '20 14:01 razzeee

Is there a workaround ?

I tried this:

If you wrap dispatch() with setTimeout() it'll be in the correct order. But that shouldn't need to be required.

But it doesn't seems to work anymore.

Decat-SimonA avatar Feb 18 '20 16:02 Decat-SimonA

Checking for an update on this issue. As mentioned above it seems as though the ngxs/devtools project is abandoned. Would be great to see some kind of resolution or potential workaround on this issue.

mdgilene avatar Sep 26 '20 02:09 mdgilene

Starting a pretty big project and trying out NGXS as my state management. I'm running into this and wondering if it'll have implications down the road. Thinking of switching to Akita but it seems convoluted... Just want something as simple as this but maintained.

DavidTheProgrammer avatar Oct 11 '20 20:10 DavidTheProgrammer

Hi @DavidTheProgrammer. I can assure you that this project is maintained ;-) The fundamental issue here is that we are "borrowing" the redux dev tools as a dev tool. Unfortunately the redux dev tools has no concept of 'asynchronous' actions (ones that have a lifecycle beyond 'dispatch'). We send the actions to the dev tool once the actions are complete, and therefore, if the first action's completion is dependent on the second action's completing then they will appear to be out of order, because this is in fact the order of completion.

We had a contributor that started writing a dev tool for us (that would recognise the action lifecycle in ngxs), but they couldn't continue due to changes in their personal life, so it hasn't received much attention since then. I made a comment here about some approaches that we could take to improve this plugin: https://github.com/ngxs/store/issues/139#issuecomment-390673126 I will raise it with the team to see what they think.

markwhitfeld avatar Oct 12 '20 12:10 markwhitfeld