Possible false positive for no-cyclic-action

[jsaguet]

The rule no-cyclic-action detects a problem in the below code.
However, since I'm using { dispatch: false } in createEffect, I am not sure the error should be raised.

Is there another way to write such an ngrx effect to comply with this rule or is this a false positive that could be fixed ?

effect$: Observable<Action> = createEffect(
    () =>
        this.actions$.pipe(
            ofType(myAction),
            tap(() => {
                this.service.showToast('Action was done');
            }),
        ),
    { dispatch: false },
);

Also, if I transform the above code into the following one to try to avoid this error, the lint process errors:

effect$: Observable<void> = createEffect(
    () =>
        this.actions$.pipe(
            ofType(myAction),
            map(() => this.service.showToast('Action was done')),
        ),
    { dispatch: false },
);

TypeError: Cannot read property 'valueDeclaration' of undefined
Occurred while linting ( line of this.actions$.pipe )
    at getActionTypes (node_modules\eslint-plugin-rxjs\dist\rules\no-cyclic-action.js:92:85)
    at checkNode (node_modules\eslint-plugin-rxjs\dist\rules\no-cyclic-action.js:72:37)
    at node_modules\eslint\lib\linter\safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (node_modules\eslint\lib\linter\safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (node_modules\eslint\lib\linter\node-event-generator.js:254:26)
    at NodeEventGenerator.applySelectors (node_modules\eslint\lib\linter\node-event-generator.js:283:22)
    at NodeEventGenerator.enterNode (node_modules\eslint\lib\linter\node-event-generator.js:297:14)
    at CodePathAnalyzer.enterNode (node_modules\eslint\lib\linter\code-path-analysis\code-path-analyzer.js:711:23)
    at node_modules\eslint\lib\linter\linter.js:952:32

[cartant]

... is this a false positive that could be fixed ?

It's not something that I'd be prepared to 'fix', as the { dispatch: false } is NgRx-specific. As it is ATM, the rule works for NgRx and redux-observable.

Essentially, a lot of the lint rules that I've written should be viewed as static analysis that's done to help developers spot potential problems - more so than rules that say "you should never do this!". If a developer has a reason for 'breaking' a rule, they should just disable it locally using one of the /* eslint-disable ... */ comments. IMO, this is an entirely reasonable thing to do.

The other alternative is to use the RxJS ignoreElements operator, like this:

this.actions$.pipe(
  ofType(myAction),
  tap(() => {
    this.service.showToast('Action was done');
  }),
  ignoreElements()
),

Also, if I transform the above code into the following one to try to avoid this error, the lint process errors:

Yeah, that's a bug. Thanks.

[jsaguet]

Yeah, that's a bug. Thanks.

👍

It's not something that I'd be prepared to 'fix', as the { dispatch: false } is NgRx-specific. As it is ATM, the rule works for NgRx and redux-observable.

Essentially, a lot of the lint rules that I've written should be viewed as static analysis that's done to help developers spot potential problems - more so than rules that say "you should never do this!".

I'll just disable the rule locally for now until the bug is fixed so that I can switch back to the map notation.

Thanks

[timdeschryver]

👋, this was something that I didn't think of while I exported this rule in ngrx-eslint.
If it causes too many problems we could see if we can expand this rule in the ngrx repo, or to write our own rule.

[cartant]

Fixed the Cannot read property 'valueDeclaration' of undefined problem. Not adding support for { dispatch: false } and decorators, etc.