Add allowThis to rxjs-no-unsafe-scope rule

[cartant]

Add allowThis to rxjs-no-unsafe-scope rule

[cartant]

I'll have to look into this - if you pardon the pun.

I have vague memories about intending to have the rule allow for method calls via this, but not property reads and writes.

I'm curious though, would your doSomething method actually use the this context in its implementation?

[kctang]

The doSomething() may be using this to access class level properties/methods. This is probably the most common use case.

Maybe something like:

class MyHttpWriter {
  constructor(private http: HttpService, private storage: StorageService) {
  }

  doSomething() {
    // get url, write to storage before returning
    return this.http.get(url).pipe(
      concatMap(json => this.doSomethingElse(json))
    )
  }

  doSomethingElse(json: string) {
    if(json) {
      return this.storage.write(json)
    } else {
      return of(null)
    }
  }
}

[cartant]

Are you absolutely sure that it's failing for method calls via this? I've added an expectation to a fixture and this.foo() does not trigger the rule. What versions of rxjs-tslint-rules, RxJS and TypeScript are you using?

I'm inclined to separate allowThis into allowMethods and allowProperties, but first I'd like to ascertain exactly what's going on and whether or not you are actually seeing the failure effected due to a method call.

[kctang]

hmm... I will need time to double confirm this. Middle of another project now. Will update here once I re-check (in a few days).

Thanks for the quick response.

[cartant]

No worries. In the interim, I will update the fixtures to include non-failing expectations that demonstrate/document the rule's behaviour with method calls.

[cartant]

See 6d3aa9c for the added expectations. Note that the method calls don't effect failures.

Once we've figured out whether or not this is the behaviour you are seeing, I'll happily add allowProperties (with a default of false) and allowMethods (with a default of true).

[kctang]

I did additional tests. With this code:

export class Demo {
  val: number

  doOkay_ReadWriteInTap () {
    this.val = 10
    return of(123, 456).pipe(tap(val => {
      console.log(this.val)
      this.val = val
    }))
  }

  doOkay_ReadInMap () {
    this.val = 10
    return of(123, 456).pipe(map(val => {
      console.log(this.val)
    }))
  }

  doNotOkay_WriteInMap () {
    this.val = 10
    return of(123, 456).pipe(map(val => {
      // next line fails linting
      this.val = val
    }))
  }

  // is similar to doNotOkay_WriteInMap() but why no linting error?
  doOkay_WriteInMapViaMethod () {
    this.val = 10
    return of(123, 456).pipe(map(val => {
      this.writeWithMethod(val)
    }))
  }

  writeWithMethod (val: number) {
    this.val = val
  }
}

This is my findings:

• The ONLY thing that cause Unsafe scopes are forbidden is this.val = val within the map() operator.
• Linting passed when it is enclosed within tap().
• Linting also passed when I used the doOkay_WriteInMapViaMethod() style which is a bit odd (feels like a workaround for doNotOkay_WriteInMap()).

With these findings, I am actually not sure if doNotOkay_WriteInMap() should or should not cause linting error. I'm a bit confused on what 'should be the best practice' now... hehe...

[cartant]

The allowDo and allowTap options default to true, so, by default, all bets are off within those operators, as they are usually only used for side effects anyway. To have the rule applied within a do or a tap, the relevant option would have to be set to false.

The rule allows method calls, but I think an allowMethods option should be added so that they, too, can be flagged.

I'm surprised that read-only use in doOkay_ReadInMap doesn't effect a failure. I'd say that's a bug.

The primary motivator for the rule was to flag the use of variables at outer scopes, but usage of this - as in doNotOkay_WriteInMap - was also something I wanted to flag, as it's a code smell, IMO.

The reason for allowing method calls was for things like AngularFire - where you might call a function that returns a database observable to which you want to switch or whatever. Or a HTTP service with which you want to call get.

I think having allowProperties default to false and allowMethods default to true is reasonable. Then the user can change the behaviour if they want.

[cartant]

The other thing that I will say is that the rules is this package don't necessarily reflect what "best practice" is. You might want to use some rules but not others.

Each rule's default configuration isn't necessarily representative of "best practice", either.

I've spoken with Ben Lesh about incorporating a subset of the rules in this package into another package to reflect what the RxJS core team considers to be best practice, but we've not yet taken that further.

[kctang]

Thanks for the clarifications on tap() and all the above. Much clearer now.

Agree that allowMethods should default to true for the reason you described. In those scenarios, would doOkay_WriteInMapViaMethod() pass the rule (writeWithMethod() access class properties)?

Look forward to having allowMethods and allowProperties options. 👍

[cartant]

Yeah, it would pass. Mutating state within a method isn't something I intend to catch with the rule. Well, not right now, anyway.

[cartant]

4.7.0 has been published and it includes the allowMethods and allowProperties options.

[kctang]

Tried using 4.7.0. Works great.

Just sharing one minor odd observation (in my code/usage). Since I am linting test code using the same config, the unsafe scopes are forbidden is showing up in unit test codes and I don't think I should fix those.

I decided to /* tslint:disable:rxjs-no-unsafe-scope */ test files with this warning because I am basically using the variables/services created during setup/teardown of the unit tests. Seems like a fair compromise.

Cheers!

[cartant]

Yes, if there are places in which you don't want the rule enforced, the approach that you've taken is the way to go. Any of TSLint's comment-based flags should be able to be applied - e.g. disable-next-line, etc.