Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

RFC: Add new Higher Order Effect - createApiEffect #1826

Closed
wesleygrimes opened this issue May 6, 2019 · 13 comments
Closed

RFC: Add new Higher Order Effect - createApiEffect #1826

wesleygrimes opened this issue May 6, 2019 · 13 comments

Comments

@wesleygrimes
Copy link
Contributor

Scenario

Most apps that use NgRx for API requests involve effects that handle a request, success and failure action.

  • The request effect listens for a request ofType and then makes an API call that returns an Observable.
  • If that Observable successfully emits a success action is returned with the Observable value as the payload.
  • If the Observable throws an error a failure action is returned with the error object as the payload.

Example using createEffect

loadRequest$ = createEffect(() =>
    this.actions$.pipe(
      ofType(load),
      startWith(load(5)),
      concatMap(action =>
        this.dataService.getItems(action.payload.count).pipe(
          map(items => loadSuccess({ items })),
          catchError(error => of(loadFailure({ error })))
        )
      )
    )
  );

Proposed Solution

Example using new createApiEffect

With this being a common pattern it might be nice to create a new type of effect that handles this scenario in a predictable way. Since createEffect is just a function, we can use composition to wrap createEffect with a new function named createApiEffect.

Let's revisit the above example using a newly imagined higher-order effect creator named createApiEffect. Our new effect would look like the following:

loadRequest$ = createApiEffect({
  actionStream: this.actions$,
  ofType: load,
  apiStream: ({ count }) => this.dataService.getItems(count),
  onSuccess: items => loadSuccess({ items }),
  onFailure: error => loadFailure({ error })
});

This provides a much cleaner visual to developers, avoids nesting, and helps prevent developers from placing operators in the wrong areas, e.g. catchError in the wrong place.

To accomplish this we will make use of the new mapToAction operator in #1822. By default, createApiEffect would use concatMap but we can pass through operator as well.

loadRequest$ = createApiEffect({
  actionStream: this.actions$,
  ofType: load,
  apiStream: ({ count }) => this.dataService.getItems(count),
  onSuccess: items => loadSuccess({ items }),
  onFailure: error => loadFailure({ error }),
  operator: mergeMap
});

Potential Implementation of createApiEffect

A first pass of createApiEffect that makes use of mapToAction would look something like this:

import { Actions, createEffect, ofType } from '@ngrx/effects';
import { ActionCreator } from '@ngrx/store';
import { Creator, TypedAction } from '@ngrx/store/src/models';
import { Observable, OperatorFunction } from 'rxjs';
import { map } from 'rxjs/operators';
import { mapToAction } from './map-to-action';

export interface CreateApiEffectConfig<T> {
  actionStream: Actions;
  ofType:
    | string
    | ActionCreator<string, Creator>
    | Array<string | ActionCreator<string, Creator>>;
  apiStream: (props) => Observable<T>;
  onSuccess: (payload: T) => TypedAction<string>;
  onFailure: (error: string) => TypedAction<string>;
  operator?: <InputAction, OutputAction>(
    project: (input: InputAction, index: number) => Observable<OutputAction>
  ) => OperatorFunction<InputAction, OutputAction>;
}

export function createApiEffect<T>(config: CreateApiEffectConfig<T>) {
  const sourceActions = Array.isArray(config.ofType)
    ? config.ofType
    : [config.ofType];

  return createEffect(() =>
    config.actionStream.pipe(
      ofType(...sourceActions),
      mapToAction({
        project: props =>
          config.apiStream(props).pipe(map(items => config.onSuccess(items))),
        error: error => config.onFailure(error),
        operator: config.operator
      })
    )
  );
}

Other information:

In addition to adding createApiEffect as a new type of effect creator, I would recommend and will assist with writing a quick guide on how to construct your own higher-order effect creators.

Solution above depends on #1822 for mapToAction.

If accepted, I would be willing to submit a PR for this feature

[x] Yes (Assistance is provided if you need help submitting a pull request)
[ ] No

@wesleygrimes wesleygrimes changed the title RFC: Add new Higher Order Effect - createApiEffect RFC: Add new Higher Order Effect - createApiEffect May 6, 2019
@Odonno
Copy link
Contributor

Odonno commented Jun 11, 2019

I am not sure if it should be about effects or actions but there is not only API calls that use this pattern of request, success and failure.

I would suggest something like createAsyncEffect or createAsyncActions depending on the feasibility of this feature. Reducing the amount of code to create 3 actions with 1 function can be a great start like that:

// create
const callApiActions = createAsyncActions('...', requestProps, successProps, failureProps);

// consume
const callApiRequestAction = callApiActions.request;
const callApiSuccessAction = callApiActions.success;
const callApiFailureAction = callApiActions.failure;

Of course, the creation of this kind of actions (meaning action types) must follow a common pattern like appending the word request, success or failure to the end of the action name.

@jonrimmer
Copy link
Contributor

jonrimmer commented Jun 20, 2019

This would be cool. We have a helper in our codebase that does something very similar. Some suggestions from our implementation:

  • Maybe call these "tasks"? E.g. createTaskEffect()
  • We automatically dispatch show / hide busy spinner actions at the beginning and end of each task, so it would be useful to expose onStart and onComplete properties as well.
  • Sometimes you want to cancel tasks, for example if the user hits the back button to navigate away from the page. We support cancellation by optionally supplying one or more cancellation actions. If supplied, we add .pipe(takeUntil(actions$.pipe(ofType(...cancels))))) to the task stream.

@wesleygrimes
Copy link
Contributor Author

@jonrimmer I like the concept of a task effect. Much more universal, I think I will refactor the above proposal to follow this convention, as well as providing a way to supply as cancel stream. That is a very common use case.

@Odonno
Copy link
Contributor

Odonno commented Aug 28, 2019

What about array deconstruction? I thought of that now and it can be an interesting idea. And from my previous post, here is what it can look like:

// create & ready to consume
const [
  callApiRequestAction,
  callApiSuccessAction,
  callApiFailureAction
] = createAsyncActions('callApi', {
  successProps: props<{ response: string[] }>(),
  failureProps: props<{ error: any }>()
});

// unionize
union({
  callApiRequestAction,
  callApiSuccessAction,
  callApiFailureAction
});

With new functions that have this definition:

const createAsyncActions = createAsyncActionsFactory({
  updateRequestActionType?, // identity function by default
  updateSuccessActionType?, // suffix 'fulfilled' at the end of every action?
  updateFailureActionType? // suffix 'failed' at the end of every action?
}); // which returns a 'createAsyncActions' function generator

createAsyncActions(baseActionType: string, {
  requestProps?,
  successProps?, 
  failureProps?
}); // which returns all 3 actions with array deconstruction

@Odonno
Copy link
Contributor

Odonno commented Aug 31, 2019

I thought about reducing code of async effect today. Here is my idea, following my previous post.

Imagine you already applied the createAsyncActionsFactory. I also made an alternative to createAsyncActions with generics:

createAsyncActions(baseActionType: string); // no props at all, seems unprobable but why not
createAsyncActions<TRequestProps>(baseActionType: string); // no props for success and failure
createAsyncActions<TRequestProps, TSuccessProps>(baseActionType: string); // no props for failure
createAsyncActions<TRequestProps, TSuccessProps, TFailureProps>(baseActionType: string);

Of course, we can still provide the definition with a config object as second parameter.

And then, here we simplify effects:

Define actions

// create & ready to consume
const callApiActions = createAsyncActions<{}, { result: string[] }, { error: any }>('callApi');

const [
  callApiRequestAction,
  callApiSuccessAction,
  callApiFailureAction
] = callApiActions;

// unionize
union({
  callApiRequestAction,
  callApiSuccessAction,
  callApiFailureAction
});

Define effects

Now, we can use the callApiActions variable which is an array with all 3 async actions (request, success, failure).

callApi$ = createAsyncEffect(
  callApiActions,
  callApiObservable,
  'merge'
);

And its type definition would be:

createAsyncEffect(
  callApiActions: AsyncActions, // the array of 3 actions
  callApiObservable: Observable<TResult>, // TResult should match the result type of the success action
  operation: 'merge' | 'concat' | 'switch' | 'concat'
);

The actions are used to filter the request action and then dispatch the result (success, failure). The observable should return the result of the async effect, it can be an API call, a retrieve/save to local-storage or any kind of observable-like operation. And finally, the operation will be used to define the rxjs operator to use on top of the observable (mergeMap, concatMap, switchMap or concatMap).

@PavelSkopik
Copy link

PavelSkopik commented Nov 14, 2019

I think that this use case can be more generic (as mentioned earlier in this thread). Instead of handling async actions/effects start, success or failure, it would be useful to have a well defined action life-cycle which NGRX tracks (e.g. dispatched, error, cancel etc.) . Then you could ask NGRX what state a certain action has and use that where needed.

I am aware of effects life-cycle but there is no implicit distinction between success and failure.

@jonrimmer
Copy link
Contributor

I think "lifecycle" is a great name for what we're talking about here, a high-level pattern for capturing a particular, common flow found across many uses of NgRx, that looks something like this:

image

I really like @Odonno's suggestion of having helper functions to create all the lifecycle actions in one go, and being able to pass them them into the effect creator. Or maybe there could be a single creator function that produces both the effect and the actions:

const { actions, effect } = createLifecycle(task, 'callApi');

@PavelSkopik
Copy link

I was thinking of taking this even a bit further where the life-cycle is “baked” into NGRX itself and the library manages that out of the box. You can just query the state (e.g. via an operator).

@jonrimmer
Copy link
Contributor

jonrimmer commented Nov 14, 2019

Yeah, that makes sense as well, so you could also do:

const { selectors, reducer } = createLifecycle(task, 'callApi');

And the reducer state would be something like:

interface LifecycleState<TResult, TError> {
  state: null | 'waiting' | 'running';
  result: TResult | null;
  error: TError | null;
  lastStatus: 'success' | 'cancel' | 'error' | null;
}

And selectors would be { getState, getResult, getError, getLastStatus }.

Although I think you'd still want to make the underlying actions and effect available, in case people wanted to customise the reducer logic, or use them to trigger additional effects.

@Odonno
Copy link
Contributor

Odonno commented Nov 14, 2019

@jonrimmer Oh my. I never thought we could go one step further. Your example of const { actions, effect } = createLifecycle(task, 'callApi'); seems really powerful.

However, I do not really understand how you will use a LifecycleState, with selectors and reducers. I can understand you will:

  • add a new property lifecycles in the root state
  • can listen to any changes to each lifecycle (to display loader component for example)

But is that a replacement of Effect?

PS: And really nice schema BTW

@jonrimmer
Copy link
Contributor

@Odonno I don't see the reducer/selectors as replacing the effect so much as automatically reflecting its opaque, internal state in a way that you can bind to within the UI. E.g if you have an effect, but don't have a reducer/state, how would you know when to show the loading component?

@Odonno
Copy link
Contributor

Odonno commented Nov 15, 2019

Well. Yes, you have to incorporate it in the root state. But, now I get it. The lifecycle will always be a layer on top of Effect. That's smart. But to make it work, you clearly need a unique identifier that will be used to store lifecycles as a key-value dictionary, like it is done with @ngrx/entity. And so you can query a single lifecycle or all the lifecycles running in the app.

@david-shortman
Copy link
Contributor

Does this align more with the goals of ngrx/data instead of ngrx/effects?

@ngrx ngrx locked and limited conversation to collaborators Jul 21, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Projects
None yet
Development

No branches or pull requests

7 participants