nartc / nartc-workspace

A collection of packages built by Nartc
MIT License
20 stars 6 forks source link

Possible Async Issue - createEffect updates slice before it completes async http operation. #10

Closed Tzvetelin88 closed 1 year ago

Tzvetelin88 commented 1 year ago

Hello, I encounter strange behaviour, not sure if this is related to the ngrx-slice library. When calling action creator which calls effect, to get data from http, it return first response to action creator, updates the slice, and then http is resolved and returned data.

I have tried few different approaches:

getPipelineLogs$ = createEffect(() =>
    this.actions$.pipe(
      ofType(DeploymentActions.getPipelineLogs),
      switchMap((currentValue) => {
        return this.http.get<any>(
          `${this.apiBase}/pipelines/${currentValue.pipelineId}/logs`,
          this.bufferHttpOptions
        )
      }),
      map((result) => {
        return DeploymentActions.setPipelineLogs({ name: result.logName, data: result.logData });
      }),
      catchError((e) => of(DeploymentActions.error(e)))
    )
  );

to add .pipe() on the request itself:

getPipelineLogs$ = createEffect(() =>
    this.actions$.pipe(
      ofType(DeploymentActions.getPipelineLogs),
      switchMap((currentValue) => {
        return this.http.get<any>(
          `${this.apiBase}/pipelines/${currentValue.pipelineId}/logs`,
          this.bufferHttpOptions
        )
          .pipe(
            map((result) => {
              return DeploymentActions.setPipelineLogs({ name: result.logName, data: result.logData });
            }),
            catchError((e) => of(DeploymentActions.error(e)))
          )
      })
    )
  );

but it aways first return response to action creator(updating slice with undefined) and then go back and resolve the http.get.

I also tried to change switchMap to exhaustMap, and different operator in the equation, but didn't make it work to await http and then continue with next operator.

Can you help on this, thanks so much!

nartc commented 1 year ago

Hi, thank you for reporting. I personally don't think it's an issue w/ ngrx-slice. But I am happy to take a look if you can provide a reproduce.

Tzvetelin88 commented 1 year ago

Thanks @nartc. I'm working on this stackblitz, still have some issue to make it run, but the code is there. I will try to make it work https://stackblitz.com/edit/angular-rdtpgn?file=src/app/app.module.ts

nartc commented 1 year ago

@Tzvetelin88 I got the Stackblitz to work here https://stackblitz.com/edit/angular-nk45eb?file=src%2Fapp%2Fapp.module.ts,src%2Fapp%2Fapp.component.ts,src%2Fapp%2Fstore%2Fdeployment.effects.ts,src%2Fapp%2Fstore%2Fdeployment.slice.ts but didn't see the issue. Can you take a look?

Tzvetelin88 commented 1 year ago

@nartc thanks for the stalblitz, not sure I messed up to make it run.

Actually the issue can bee seen there: When we click on the button we need to get the order:

1. "getPipelineLogs clicked ..."
2. "Executing createEffect for getPipelineLogs ..."
3. Then we need to get "Executing createEffect for getPipelineLogs..."
4. "Executing createEffect for setPipelineLogs..."  -> (we make sure "createEffect" ) completes and then updating slice
6. "Inside Slice, executing setPipelineLogs..."  update slice
7. "Inside Slice, executing setPipelineLogs done"  update slice

flow should be -> createEffect is a async operation that we need to wait to complete, then update slice, and after this.store.dispatch completes, to used the data, yes?

nartc commented 1 year ago

@Tzvetelin88 You have the same console.log there in both switchMap and map. If you differentiate them, then you'll see this instead

image

You can see that inside map --> log, we have the result from the http.get() above. Can you double check the updated stackblitz https://stackblitz.com/edit/angular-nk45eb?file=src%2Fapp%2Fapp.module.ts,src%2Fapp%2Fapp.component.ts,src%2Fapp%2Fstore%2Fdeployment.effects.ts,src%2Fapp%2Fstore%2Fdeployment.slice.ts ?

Also, this.store.dispatch() is a sync operation. You cannot console.log right after this.store.dispatch() and expects a guaranteed value because the Action being dispatched might trigger an Effect (which it does in our case).

console.log(1);
Promise.resolve().then(() => {
  console.log(2);
});
console.log(3);

// logs: 1, 3, 2

Same concept here.

Tzvetelin88 commented 1 year ago

I see. What options are there to still usethis.store.dispatch() with Actions which triggering Effect, that we need to await, to update the slice and then to rely on the new state, that we can use on UI components?

nartc commented 1 year ago

It is best to have everything flow through your reducer.

Action dispatched --> Effect triggered --> Effect executed --> Effect dispatched another Action to update State --> State updated --> Selector emits

You should always update state via Reducer and read state via Selector.

Tzvetelin88 commented 1 year ago

Can you please help me to do this in the stakblitz example, so I can check the logic and inherit it for feature http async effects, and their proper await, update state and display up to date state in the UI

Tzvetelin88 commented 1 year ago

Actually, no need. I was looking at wrong direction. I thing I need to take a break from work :D. I was managed to follow up how SAGA work.Sorry that I lost your time :(