angular-redux / store

Angular 2+ bindings for Redux
MIT License
1.34k stars 205 forks source link

UI not updated after epic actions #455

Open tristanmkernan opened 6 years ago

tristanmkernan commented 6 years ago

This is a...

What toolchain are you using for transpilation/bundling?

Environment

NodeJS Version: 8.2.1 Typescript Version: 4.4.2 Angular Version: 4.3.2 @angular-redux/store version: 6.5.7 @angular/cli version: (if applicable) 1.2.6 OS: OS X, Ubuntu

Link to repo showing the issues

private, happy to share as much code as possible tho

Expected Behaviour:

UI updates after action.

Actual Behaviour:

UI does not update after action.

Additional Notes:

this happens for actions that are two levels deep in epics (redux-observable, as recommended by this repo), for example I have one epic that asynchronously emits another action which is picked up by another epic, and it is the actions emitted by the second epic that do not result in a UI change until I click somewhere.

I have played around with NgZone to no avail, using it literally everywhere in the epics. I followed the conversation in #259 by checking NgZone.isInAngular() everywhere but even when I could confirm the epics all worked in Angular, nada!

the ui is listening thru the @select decorator, and i know that works because i tried subscribing to this and the results are output to console while the UI is not updated.

e-schultz commented 6 years ago

Can you give an example of your reducers? if you have accidentally mutated a slice of state instead of returning a new copy - it can cause change detection to not pick things up when you expect.

tristanmkernan commented 6 years ago

thanks for the response @e-schultz . you were right, that the state was mutated, however i updated it to create a new object and still no success. when the login error is returned as a result of an Observable catch method, the UI is not updated until I click.

export const errorReducer: Reducer<IErrorStore> = (
    previousState: IErrorStore = ERROR_STORE_INITIAL_STATE, action: Action<Error | string>
): IErrorStore => {
    const nextState: IErrorStore = Object
        .keys(previousState)
        .reduce(
            (acc, key) => acc[key] = previousState[key], {}
        );

    switch (action.type) {
    case ErrorActions.REMOVE:
        nextState[<string> action.payload] = null;

        return nextState;
    }

    if (!action.error) {
        return nextState;
    }

    switch (action.type) {
    case LoginActions.USER_LOGGED_IN_ERROR:
        nextState.login = <Error> action.payload;

        return nextState;
    default:
        return nextState;
}
e-schultz commented 6 years ago

Where in your chain is the catch being attached? looking at the redux-observable docs on error handling - if you put a catch on the .ofType - it can kill the stream, which may be causing a problem.

tristanmkernan commented 6 years ago

i think it is correct. here is my epic:

    login = (action$) => {
                return action$.ofType(LoginActions.DO_LOGIN)
                    .mergeMap(
                        () => {
                            const state = this.ngRedux.getState();

                            const headers = new Headers({
                                'Authorization': `Bearer ${state.auth.jwt}`
                            });

                            return this.http
                                .get(`${state.config.apiUrl}/me`, {headers})
                                .map(
                                    response => {
                                        return {type: LoginActions.USER_LOGGED_IN, payload: response.json().data};
                                    }
                                )
                                .catch(
                                    err => {
                                        /*
                                          Todo: why does returning an action naturally not
                                          update the UI?
                                        */
/*
                                        this.errorActions.add({
                                            type: LoginActions.USER_LOGGED_IN_ERROR,
                                            payload: new Error('Failed to login.'),
                                            error: true
                                        });
*/

                                        return [{
                                            type: LoginActions.USER_LOGGED_IN_ERROR,
                                            payload: new Error('Failed to login.'),
                                            error: true
                                        }];
                                    }
                                );
                        }
                    );
    }
e-schultz commented 6 years ago

Hi,

Was just playing around with an epic in the example repo - and didn't have any issues with the action returned from a .catch updating the state, although wondering if there could be issues with: payload: new Error('Failed to login.'), intead of simply payload: 'Failed to Login'

login = action$ => {
  return action$.ofType(LoginActions.DO_LOGIN).mergeMap(() => {
    const state = this.ngRedux.getState();

    const headers = new Headers({
      Authorization: `Bearer ${state.auth.jwt}`
    });

    return this.http
      .get(`${state.config.apiUrl}/me`, { headers })
      .map(response => {
        return {
          type: LoginActions.USER_LOGGED_IN,
          payload: response.json().data
        };
      })
      .catch(err => {
        return [
          {
            type: LoginActions.USER_LOGGED_IN_ERROR,
            payload: new Error('Failed to login.'),
            error: true
          }
        ];
      });
  });
};

This should work.

However, if you had:

class Something {
  login = action$ => {
    return action$.ofType(LoginActions.DO_LOGIN).mergeMap(() => {
      const state = this.ngRedux.getState();
      const headers = new Headers({
        Authorization: `Bearer ${state.auth.jwt}`
      });

      return this.http
        .get(`${state.config.apiUrl}/me`, { headers })
        .map(response => {
          return {
            type: LoginActions.USER_LOGGED_IN,
            payload: response.json().data
          };
        })
        .catch(err => {
        // this catch isn't returning any observables or any values - so the stream dies out
        // and could be causing other things to not trigger

          this.errorActions.add({
            type: LoginActions.USER_LOGGED_IN_ERROR,
            payload: 'Failed to login.'
            error: true
          });
        });
    });
  };
}

Tweaked the epic/service locally in my example to force a failure:

private createLoadAnimalEpic(
    animalType: AnimalType
  ): Epic<AnimalAPIAction, IAppState> {
    return (action$, store) =>
      action$
        .ofType(AnimalAPIActions.LOAD_ANIMALS)
        .filter(action => actionIsForCorrectAnimalType(animalType)(action))
        .filter(() => animalsNotAlreadyFetched(animalType, store.getState()))
        .switchMap(() =>
          this.service
            .getAll(animalType)
            .map(data => this.actions.loadSucceeded(animalType, data))
            .catch((response: any) => {
              return [
                this.actions.loadFailed(animalType, {
                  status: '' + response.status
                })
              ];
            })
            .startWith(this.actions.loadStarted(animalType))
        );
  }

and didn't have an issue from that.

tristanmkernan commented 6 years ago

right, it actually does work in other epics, what i had meant in the OP (and subsequently forgotten until now now...) was that this epic is invoked by another epic. basically i have one epic that loads the initial state, and if the user is logged in, then retrieve the user, which triggers the epic in question.

furthermore, i ask this because catch()has to return an action, so in my real code below, i have to return a fake action. also note how i need to use ngZone.run() to use the router, because the UI does not update otherwise (despite the console showing the correct redux state)

                            return this.http
                                .get(`${state.config.apiUrl}/me`, {headers})
                                .map(
                                    response => {
                                        let target: string;

                                        if (state.config.displayOnOpen) {
                                            target = '/draw-options/viewer';
                                        } else {
                                            target = '/draw-options/image-controls';
                                        }

                                        this.ngZone.run(
                                            () => this.router.navigateByUrl(target)
                                        );

                                        return {type: LoginActions.USER_LOGGED_IN, payload: response.json().data};
                                    }
                                )
                                .catch(
                                    err => {
                                        /*
                                          Todo: why does returning an action naturally not
                                          update the UI?
                                        */

                                        this.errorActions.add({
                                            type: LoginActions.USER_LOGGED_IN_ERROR,
                                            payload: new Error('Failed to login.'),
                                            error: true
                                        });

                                        return [{
                                            type: 'IGNORE_ME'
                                        }];
                                    }
                                );
e-schultz commented 6 years ago

Hey, just wondering if this is still an issue? been falling behind on some things - if you still need help I can leave this open

tristanmkernan commented 6 years ago

@e-schultz yes this is still an issue. if you have any pointers i'm happy to try and help out and find the fix but i'm out of ideas 😢