SAP / spartacus

Spartacus is a lean, Angular-based JavaScript storefront for SAP Commerce Cloud that communicates exclusively through the Commerce REST API.
Apache License 2.0
733 stars 379 forks source link

Cant access API failure message #7254

Open Ross-Rawlins opened 4 years ago

Ross-Rawlins commented 4 years ago

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

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

Let's continue here

seanheinen commented 4 years ago

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 commented 4 years ago

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.

seanheinen commented 4 years ago

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!