feat(effects): allow non-dispatching effects to not return an action #1689
Conversation
|
Preview docs changes for ead9718 at https://previews.ngrx.io/pr1689-ead9718/ |
| @@ -4,14 +4,14 @@ import { EffectMetadata } from './models'; | |||
|
|
|||
| const CREATE_EFFECT_METADATA_KEY = '__@ngrx/effects_create__'; | |||
|
|
|||
| export function createEffect<T extends Action>( | |||
| source: (() => Observable<T>), | |||
There was a problem hiding this comment.
We have quite a few overloads, I think we can get away with just 3:
export function createEffect<
R extends Observable<unknown> | ((...args: any[]) => Observable<unknown>)
>(source: () => R, options: { dispatch: false }): R;
export function createEffect<
T extends Action,
R extends Observable<T> | ((...args: any[]) => Observable<T>)
>(source: () => R, options?: { dispatch: true }): R;
export function createEffect<
T extends Action,
R extends Observable<T> | ((...args: any[]) => Observable<T>)
>(source: () => R, { dispatch = true } = {}): R {
const effect = source();
Object.defineProperty(effect, CREATE_EFFECT_METADATA_KEY, {
value: {
dispatch
},
});
return effect;
}|
|
||
| expect(effect['__@ngrx/effects_create__']).toEqual({ dispatch: false }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Could you please add a test with expectSnippet to catch that createEffect(() => of('foo')) /is not assignable to type 'Observable/ ?
I think it could be beneficial - right not nothing prevents from accidentally adjusting the code to return Observable<any>
|
I'll address @alex-okrushko 's feedback |
|
Preview docs changes for ccf611e at https://previews.ngrx.io/pr1689-ccf611e/ |
|
@timdeschryver changes LGTM, but its failing in CI with the snippet checker. |
|
Yep thanks, I couldn't figure out why the CI is failing yesterday. I'll take a second look with a fresh mind later today. |
|
Preview docs changes for 0013159 at https://previews.ngrx.io/pr1689-0013159/ |
|
Preview docs changes for de12acb at https://previews.ngrx.io/pr1689-de12acb/ |
|
Preview docs changes for d4b7d79 at https://previews.ngrx.io/pr1689-d4b7d79/ |
1 similar comment
|
Preview docs changes for d4b7d79 at https://previews.ngrx.io/pr1689-d4b7d79/ |
timdeschryver
force-pushed
the
pr/effect-no-dispatch
branch
from
d4b7d79 to
121b6c2
Compare
|
Preview docs changes for 121b6c2 at https://previews.ngrx.io/pr1689-121b6c2/ |
1 similar comment
|
Preview docs changes for 121b6c2 at https://previews.ngrx.io/pr1689-121b6c2/ |
|
Seems like I'm not able to solve the bazel tests 😅 |
|
I'll take a look |
|
Preview docs changes for 88da3a2 at https://previews.ngrx.io/pr1689-88da3a2/ |
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?
Follow up to #1667
What is the new behavior?
Does this PR introduce a breaking change?
Other information