ngrx / platform

Reactive State for Angular
https://ngrx.io
Other
8k stars 1.97k forks source link

false positives/negatives for rule @ngrx/no-multiple-actions-in-effects when using conditionals #4421

Closed P4 closed 2 months ago

P4 commented 2 months ago

Which @ngrx/* package(s) are the source of the bug?

eslint-plugin

Minimal reproduction of the bug/regression with instructions

const condition = true;
const foo = () => ({type: 'foo'}) as const,
  bar = () => ({type: 'bar'}) as const;

createEffect(
  // False positive - single action, reported as multiple
  () => inject(Actions).pipe(switchMap(() => (condition ? of(foo()) : of(bar())))),
  {functional: true},
);

createEffect(
  // False negative - two arrays of actions, not reported
  () => inject(Actions).pipe(switchMap(() => (condition ? [foo(), foo()] : [bar(), bar()]))),
  {functional: true},
);

Expected behavior

In the first effect, conditional expression returns one of two actions - the type is a union. Only one action actually gets emitted, so it should not be flagged

The second effect returns one of two arrays of multiple actions, so it should be reported as a problem by the rule.

Versions of NgRx, Angular, Node, affected browser(s) and operating system(s)

NgRx: 18.0.1 Angular: 18.0.6 Node: 18.20.2

Other information

Possibly caused by https://github.com/ngrx/platform/blob/377f3b832f845b984fe964d8e3d1f099fdae4963/modules/eslint-plugin/src/rules/effects/no-multiple-actions-in-effects.ts#L59-L63

The case for unions seems to be checking whether one of the types is not an array, when it should be doing the opposite? Hence the observed behavior - union with zero array types is flagged but union of only arrays is not.

I would be willing to submit a PR to fix this issue

P4 commented 2 months ago

Looks like it may have already been resolved by https://github.com/ngrx/platform/pull/4418 (thanks @jsaguet!)