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

feat(effects): allow ofType to handle ActionCreator #1676

Merged
merged 2 commits into from
Apr 1, 2019
Merged

feat(effects): allow ofType to handle ActionCreator #1676

merged 2 commits into from
Apr 1, 2019

Conversation

alex-okrushko
Copy link
Member

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Closes #

What is the new behavior?

Allows to filter Actions with ofType(actionCreator) as well as maintains previous behavior ofType(actionCreator.type).

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@ngrxbot
Copy link
Collaborator

ngrxbot commented Mar 31, 2019

Preview docs changes for 8549f84 at https://previews.ngrx.io/pr1676-8549f84/

allowedTypes.some(type => type === action.type)
);
return filter((action: Action) => {
for (let typeOrActionCreator of allowedTypes) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about checking on action first and next on the action creator type?

return allowedTypes.some(type => {
  // Comparing the string to type
  if (typeof type === 'string') {
    return type === action.type;
  }

  // We are filtering by ActionCreator
  return type && type.type === action.type;
});

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, reads much better. Also I think I reimplemented some in my previous solution 🤦‍♂️

@ngrxbot
Copy link
Collaborator

ngrxbot commented Apr 1, 2019

Preview docs changes for 531d115 at https://previews.ngrx.io/pr1676-531d115/

Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM

Right now this offers type safety for 5 action types.
This was due to the fact that we had to provide the typings.
If we want, we could add new typings as in the ts-action-operators library of Nicholas.
See https://github.com/cartant/ts-action-operators#oftype for ref.

 ofType({ foo, bar }),
  tap(action => {
    // Here, the action has been narrowed to `typeof union({ foo, bar })`.
    // Common properties will be accessible, other will require further narrowing.
    if (isType(action, foo)) {
      // Here, the action has been narrowed to a FOO action.
    } else if (isType(action, bar)) {
      // Here, the action has been narrowed to a BAR action.
    }
  })

@brandonroberts brandonroberts merged commit a41d1d6 into ngrx:master Apr 1, 2019
brandonroberts pushed a commit that referenced this pull request Apr 4, 2019
@alex-okrushko alex-okrushko deleted the typeof branch October 30, 2019 03:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants