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

Cant access API failure message #7254

Open
Ross-Rawlins opened this issue Apr 14, 2020 · 6 comments
Open

Cant access API failure message #7254

Ross-Rawlins opened this issue Apr 14, 2020 · 6 comments

Comments

@Ross-Rawlins
Copy link

This template is to be used for bug reports and small enhancements. For feature requests, please contact the project owner.

Note: For the bug to be accepted, it must be reproducible using the latest release of Spartacus. See our Contributor Documentation for more information.

Environment Details

  • Spartacus: 1.4.1
  • Browser: Not Applicable
  • OS: Not Applicable
  • Device: Not Applicable
  • Other environment details: Not Applicable

Steps to Reproduce

Do an API call with any of the NGRX stores

Observed Results

API call will return a true/false or just the error code.

Expected Results

Get returned error message from the server

Repository Used

This occurs in the current repo.

Offer to Fix

We found a fix in the loader reducer but this could be set this way for a specific reason. Please let me know if this could be added.

loader.reducter.ts:45
change this to value: reducer ? reducer(state.value, action) : action.payload,

Additional Information

@marlass
Copy link
Contributor

marlass commented Apr 16, 2020

@Ross-Rawlins
This is the desired behavior of our reducers. You could always provide your own reducer.
And if you don't have access to the error in action then let us know which particular actions lack the details in the payload.

@Ross-Rawlins
Copy link
Author

Ross-Rawlins commented Apr 16, 2020

@marlass I see you replied in SO and here. Would you like to keep the communication here or there? I want to tell my team what thread to watch.

@marlass
Copy link
Contributor

marlass commented Apr 16, 2020

Let's continue here

@seanheinen
Copy link

seanheinen commented Apr 16, 2020

Hi there @marlass,

Thanks for this response. The feedback definitely helps.

If I'm understanding you correctly your advice would be relevant for using the loaderReducer with our custom ngrx stores?

Our issue had arose once we began using some of the Spartacus actions, namely the UserActions.UpdateUserDetails. From my understanding we're not able to extend the user-details reducer as they're simply functions? That and the fact that I don't think dabbling in Spartacus' store would be the right approach anyway.

If you're perhaps aware of any way to extend/override the Spartacus reducers though, that would be first prize?

Our current workaround in the meantime is to create a more primitive process store along side that of Spartacus' and manage anything extra there i.e. error payloads.

Our first stab at it would be to subscribe to any StateEntityLoaderActions and action to our Process store accordingly. This way we're able to grab anything from the meta fields too - we're aware that not all Actions extend the StateEntityLoaderActions, though we're hoping it should cover most of our use case.

Example:

@Injectable({ providedIn: "root" })
export class ProcessEffects {

    public readonly load$ = createEffect(() => this.actions$.pipe(
        filter(isLoadAction),
        map((action) => action.meta.entityId as string),
        mergeMap((id) => of(ProcessAction.load({ id })))
    ));

    public readonly success$ = createEffect(() => this.actions$.pipe(
        filter(isSuccessAction),
        map((action) => action.meta.entityId as string),
        mergeMap((id) => of(ProcessAction.success({ id })))
    ));

    public readonly error$ = createEffect(() => this.actions$.pipe(
        filter(isErrorAction),
        mergeMap((action) => of(ProcessAction.error({
            id: action.meta.entityId as string,
            error: action.meta.loader.error
        })))
    ));

    public readonly reset$ = createEffect(() => this.actions$.pipe(
        filter(isResetAction),
        map((action) => action.meta.entityId as string),
        mergeMap((id) => of(ProcessAction.reset({ id })))
    ));

    public constructor(private readonly actions$: Actions) { }

}

function isLoadAction(action: Action): action is StateEntityLoaderActions.EntityLoadAction {
    return action instanceof StateEntityLoaderActions.EntityLoadAction;
}
function isSuccessAction(action: Action): action is StateEntityLoaderActions.EntitySuccessAction {
    return action instanceof StateEntityLoaderActions.EntitySuccessAction;
}
function isErrorAction(action: Action): action is StateEntityLoaderActions.EntityFailAction {
    return action instanceof StateEntityLoaderActions.EntityFailAction;
}
function isResetAction(action: Action): action is StateEntityLoaderActions.EntityResetAction {
    return action instanceof StateEntityLoaderActions.EntityResetAction;
}

This approach has looked promising with our initial tests. Does it raise any concerns from your end?

Thanks in advance.

EDIT:
I've just noticed that we should be able to override the Spartacus reducers?
By implementing our own factory for the reducerToken.

export const reducerProvider: Provider = {
  provide: reducerToken,
  useFactory: getReducers,
};

@marlass
Copy link
Contributor

marlass commented Apr 17, 2020

Hi,
Handling that in effects is the way I would do this as well at the moment. I would avoid overriding reducers because any change we do there you would need to replicate in your own reducer.

In 2.0 we will release mechanism for event system and that would be my preferred mechanism to solve problems with error handling. You would need to create your own event that will be based on existing action and then you could listen to that event in service/component and in there handle some reloads/errors on fail action.
In 2.0 we won't have a lot of events, so that's why you would need to create that one.

@Xymmer Xymmer added this to the 2.x milestone Apr 20, 2020
@seanheinen
Copy link

Thanks for the response. That's good to hear - looking forward to see your approach.

I've done a bit more experimentation with our effects and it seems like it's going to be the most scalable way going forward till the mentioned change.

Should we need our effects to cast a wider net we can simply discriminate/infer based on your meta fields for now.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants