-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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, Example): add createEffect #1667
Conversation
Preview docs changes for 47991c3 at https://previews.ngrx.io/pr1667-47991c3/ |
47991c3
to
6671dbf
Compare
Preview docs changes for 6671dbf at https://previews.ngrx.io/pr1667-6671dbf/ |
6671dbf
to
36a4eeb
Compare
Preview docs changes for 36a4eeb at https://previews.ngrx.io/pr1667-36a4eeb/ |
1 similar comment
Preview docs changes for 36a4eeb at https://previews.ngrx.io/pr1667-36a4eeb/ |
const CREATE_EFFECT_METADATA_KEY = '__@ngrx/effects_create__'; | ||
|
||
export function createEffect<T extends Action>( | ||
source: (() => Observable<T>), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be Observable for the dispatch false? We shouldn't force it to be an Action for dispatch: false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had it first as a non action but it seemed a bit weird to me while changing the example-app,
because the non-dispatching effects were all tap
s on the actions stream.
But now that you mention it, I do agree. It would make an effect harder to implement on a non-actions stream.
What do you think @brandonroberts ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree also. I thought the same, but I didn't mention it in the review 😞
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Closes #1368
What is the new behavior?
Does this PR introduce a breaking change?
Other information