cartant / eslint-plugin-rxjs

ESLint rules for RxJS
MIT License
312 stars 37 forks source link

no-unsafe-takeuntil: Accept object properties as operators #90

Closed alwonder closed 2 years ago

alwonder commented 2 years ago

Motivation

I wanted to add the rule for the custom takeUntilDestroy operator in our working project. The operator is the method of an angular component, which fires when ngOnDestroy is called. But I stumbled upon the issue that the rule ignores my method:

// .eslintrc.json

"rxjs/no-unsafe-takeuntil": [
  "error",
  {
    "alias": ["takeUntilDestroy"]
  }
]
// My method

of(null)
  .pipe(
    this.takeUntilDestroy(), // It doesn't say anything
    tap(() => {}),
  )
  .subscribe(() => {});

Solution

I added the new option acceptObjectProperties (false by default) which allows adding not only functions but object properties and class members. So with this config:

// .eslintrc.json

"rxjs/no-unsafe-takeuntil": [
  "error",
  {
    "alias": ["takeUntilDestroy"],
    "acceptObjectProperties": true
  }
]

eslint would highlight the problem method:

// My method

of(null)
  .pipe(
    this.takeUntilDestroy(), // Applying operators after takeUntil is forbidden.
    tap(() => {}),
  )
  .subscribe(() => {});
cartant commented 2 years ago

I think we could make this change without introducing the acceptObjectProperties option. That is, checking for a call via a member expression would be sensible default behaviour - as calls made via namespaced imports are represented as member expressions.

ATM - i.e. without your change - this wouldn't behave the way it should:

import * as rxjs from "rxjs";
rxjs.of(null)
  .pipe(
    rxjs.takeUntil(rxjs.NEVER),
    rxjs.tap(() => {}),
  )
  .subscribe(() => {});

So your change is more of a general fix, I think.

alwonder commented 2 years ago

Yes, I think you're right. In that case, I'll delete the option.

cartant commented 2 years ago

Your change should be in 4.0.4 - which has just been published.

alwonder commented 2 years ago

Great, thanks!