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

Extensibility for Action Handlers... (Action Pipes?) #192

Open
markwhitfeld opened this issue Mar 29, 2018 · 12 comments
Open

Extensibility for Action Handlers... (Action Pipes?) #192

markwhitfeld opened this issue Mar 29, 2018 · 12 comments
Milestone

Comments

@markwhitfeld
Copy link
Member

markwhitfeld commented Mar 29, 2018

Challenge

In order to preserve the simplicity of ngxs I think that it would be great to have an extensibility point for action handlers so that some of the more complex things can be abstracted into helpers.
Having used ngrx on a large project I have seen how developers struggle with ngrx a great deal due to having to learn rxjs. It would be great if it would be possible to implement a relatively complex solution using ngxs without having to resort rxjs to handle certain scenarios.
The cancellation of the previous request to prevent race conditions is just one example.
There is currently an open issue (#191) with a proposal to simplify things for cancellation using an option parameter for the @action decorator. This is the same as what I proposed as an alternative in the description of issue #143, so I think it is on the right track.
What I am proposing here is an extension of that idea that allows for extensibility.

Proposal

This proposal is essentially a copy of my comments (here and here) on issue #143.

What I am thinking is that we need to be able intercept and control the execution, inputs and outputs of the action handler. This sounds like a Chain of Responsibility design pattern to me. Although, the actual implementation detail should have no bearing on its usage.

We could have the ability to add classes into the pipeline that processes the request.
These could be called one of the following: ActionFilter, ActionInterceptor, ActionHandler, ActionPipe
I think that I prefer ActionPipe because it fits that paradigm so I will use that word going forward.
I propose that the code could look like this:

@Action(FeedAnimals, [ LatestCallWins ])
feedAnimals({ getState, setState }: StateContext<ZooStateModel>, { payload }: FeedAnimals) { 
  ...
  }

The Action Pipes could be expressed as an array @Action(FeedAnimals, [LatestCallWins, ...]) or as extra parameters @Action(FeedAnimals, LatestCallWins, ...).
I deliberately tried to keep the LatestCallWins name expressive but it could also be Cancellable, SwitchMap, CancelsPrevious, LatestCallCancelsPrevious or similar. You would be able to list multiple Action Pipes in the same fashion as providers on a module (by type, by factory or by value).

Regarding the proposed solution for cancellation in issue #191, this could fit into the action pipe pattern in the following way (although the Options pipe could be inferred too):

@Action(FeedAnimals, Options({ cancelable: true }))
feedAnimals(...) {   ...  }

Options would just be another Action Pipe that handles the action according to the options specified.

We could define ones for Debouncing, Throttling, Transformations, and Extensions.
The process function could even look different if the ActionPipe is able to call it.
For Example you could have:

@Action(FeedAnimals, [ ActionMap ])
feedAnimals({ payload }: FeedAnimals) { 
    return [ new FeedCows(payload.hay), new FeedChickens(payload.seeds) ];
  }

Here the ActionMap pipe would expect a function that returns one or many actions and it would dispatch those automatically.

I'm quite excited about the prospects of this! Hopefully I will get a chance to create a pull request for this soon (I believe that this should be relatively easy to acheive), unless somebody beats me to it... or unless this idea gets shot down ;-)

@markwhitfeld
Copy link
Member Author

Adding some more previous thoughts here...
I think that this approach can be really useful as an extensibility point. As an example of this, here are some other plugin possibilities that I have thought of if we enable this Action Pipe approach:

Utility

AsReducer

@Action(FeedAnimals, [ AsReducer ])
feedAnimals(state: ZooStateModel, { payload }: FeedAnimals) { 
  return { ...state, feed: payload };
}

AsStatePatch

@Action(FeedAnimals, [ AsStatePatch ])
feedAnimals({ payload }: FeedAnimals) { 
  return { feed: payload };
}

AsActionStream (or AsObservable)

@Action(FeedAnimals, [ AsActionStream ])
feedAnimals(state: StateContext<ZooStateModel>, source$: Observable<FeedAnimals>) { 
  return source$.pipe(
      switchMap(({ payload }) => ...)
    );
}

Entity Loader Plugin

EntityLoader

@Action(LoadAnimals, [ LatestCallWins, EntityLoader<AnimalLoadApi>])
loadAnimals(state: ZooStateModel, action: LoadAnimals, entities: Animal[]) { 
  return { ...state, animals: apiResponse};
}

NgRx Migration Plugin

AsNgRxReducer (the same as AsReducer but provided to be explicit about how the ngrx one translates)

@Action(null, [ AsNgRxReducer ])
feedAnimals(state: ZooStateModel, action: Action) { 
  return { ...state, feed: payload };
}

or reference the ngrx reducer directly because the signature is the same

@Action(FeedAnimals, [ AsNgRxReducer ])
feedAnimals = someImport.existingNgrxReducerFn;

AsNgRxEffect

@Action(FeedAnimals, [ AsNgRxEffect ])
feedAnimals(actions$: Actions) { 
  return actions$
    .switchMap( ({payload}) => 
      feedApi
        .doFeeding(payload)
        .map((result) => new FeedingDone(result) )
     );
}

or directly adapt existing ngrx effects (could even do a type alias for this action decorator as @Effect)

@Action(null, [ AsNgRxEffect ])
feedAnimals$ = actions$
    .ofType<FeedAnimals>()
    .switchMap( ({payload}) => 
      feedApi
        .doFeeding(payload)
        .map((result) => new FeedingDone(result) )
     );

This would even handle the case where something else triggers the action (like a timer) as @stupidawesome raised here:

@Action(null, [ AsNgRxEffect ])
feedAnimals$ = Observable
    .interval(24 * 60 * 60 * 1000) // 24 hours
    .map(() => new FeedAnimals());

@markwhitfeld
Copy link
Member Author

I would really love to hear people's thoughts on this.
I believe it is possible to achieve (I even think that I can get the typings working in such a way that it will pick up a mismatch in the decorated method definition!), but I want to know if this measures up to the goal of improving simplicity, readability and providing a reasonable extension point.

@deebloo
Copy link
Member

deebloo commented Mar 29, 2018

I think it is a good idea. However I have some concerns.

The big one being that we can't change the type signature of a method based on the decorator.

This could be a way to more elegantly handle the cancelable thing. we would also need to define the api of what one of these action pipes looks like.

If we keep the same type signature for the methods and limit the scope of what these things can do I am more open to it.

@amcdnl if this is something we want to consider we would need to do it before the cancelable option

@amcdnl
Copy link
Member

amcdnl commented Mar 29, 2018

So after discussion w/ @markwhitfeld , we concluded this:

  • Changing method signature not a great idea but adding to the parameters not so bad
  • ActionPipes would operate like AOP functions, lets create a new issue for that
  • The second parameter of action decorators would accept a object or array, inferring which to use
  • Action pipes should probably just be pure methods, DI is so painful
  • You can use both action pipes and options { cancellable: true, pipes: [] } like this

@deebloo
Copy link
Member

deebloo commented Mar 29, 2018

I think that makes sense. I'd say no to inferring if it is an array or an object. just leads to more documentation and more potential confusion. Also would it be possible to reuse angular Pipes? They are a simple class with a transform method that can accept any arguments and return anything

@amcdnl
Copy link
Member

amcdnl commented Mar 31, 2018

@deebloo - Well inference like that angular forms does it, so i think its something to people are familiar w/.

@markwhitfeld
Copy link
Member Author

markwhitfeld commented Apr 2, 2018

I was thinking what the API of one of these Action Pipes could look like.
(following on from @deebloo's idea of doing something similar to the Angular Pipes)

The tricky thing is that the observable returned from the handler is not the only output. The setState and the dispatch on the StateContext are also outputs. All of these need to be part of the output stream so that we have full control of all outputs.
We would need to represent this in a type for the returned ActionPipe observable.
This could be an object wrapper with a property for each of these outputs. Like this:

interface ActionOutput {
  newState?: any;
  actionToDispatch?: any;
  completedAction?: any;
}

Then the ActionPipe interface could then look like this:

interface ActionPipe {
  transform(
      action$: Observable<any>,
      next: (action$: Observable<any>) => Observable<ActionOutput>
    ): Observable<ActionOutput>;
}

As an example the CancelPrevious action pipe could look like this:

class CancelPrevious implements ActionPipe {
  transform(
      action$: Observable<any>,
      next: (action$: Observable<any>) => Observable<ActionOutput>
    ): Observable<ActionOutput> {
      return action$.pipe(
          switchMap((action) => next(Observable.of(action)))
        );
    }
}

PS. I have not done anything towards the idea of changing method signatures but I believe that that could be introduced quite simply when needed.

@markwhitfeld
Copy link
Member Author

markwhitfeld commented Apr 3, 2018

Note that in the next argument in the transform method is pipeable! By design 😄
So for example a Debounce pipe could be implemented like this:

class Debounce implements ActionPipe {

  constructor(private duration: number) { }

  transform(
      action$: Observable<any>,
      next: (action$: Observable<any>) => Observable<ActionOutput>
    ): Observable<ActionOutput> {
      return action$.pipe(
          debounceTime(this.duration),
          next
        );
    }
}

I am also aware of the idea that the ActionPipe interface could rather be for the transform method rather than a class. This could look like this:

type ActionPipe = (
    action$: Observable<any>,
    next: (action$: Observable<any>) => Observable<ActionOutput>
  ) => Observable<ActionOutput>;

function Debounce(duration: number) {
    return <ActionPipe> function (action$, next) {
        return action$.pipe(
            debounceTime(duration),
            next
          );
      };
}

I like the look of this more functional approach, but I think this breaks the Angular Pipes type of paradigm that was suggested we could follow. This is closer to an rxjs operator type of function creator.

I am not attached to this either way. I will use whatever is best to ensure the best user API (and type completion and checking) over convenience for the action handler plugin implementer API.

@amcdnl
Copy link
Member

amcdnl commented Apr 4, 2018

@markwhitfeld - Can we list out all the use cases for this we can think of? Right now we have:

  • Cancel Previous Request
  • Debounce Request

What else could we use this for? I like where its going!!!

You had an example of EntityLoader, I'm curious what that implementation would look like.

@amcdnl amcdnl added this to the 2.2.0 milestone Apr 4, 2018
@amcdnl
Copy link
Member

amcdnl commented May 8, 2018

Closing and marking for future.

@splincode
Copy link
Member

@markwhitfeld what do you think move it issue to https://github.com/ngxs/store/discussions?

@sokolej79
Copy link

I think that this is mandatory feature to have choice for decide when to use switchmap, mergemap, concatmap, exhaustmap.
Every serious state management should support this feature because of race conditions.

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

5 participants