New rule to disallow assigning to outside scopes

[aboyton]

Presently there is a rule rxjs-no-unsafe-scope which disallows using variables from an outside scope inside an operator callback. I think it would be useful to also have a similar rule that disallows assigning to an outside scope from inside a callback. In my experience this is often a code smell and can lead to strange bugs. Instead I try and make all class variables either observables or non-observables for a particular class.

For example, to take the example from https://medium.com/@paynoattn/3-common-mistakes-i-see-people-use-in-rx-and-the-observable-pattern-ba55fee3d031

initialize() {
  let id;
  this.appParameters.subscribe(params => id = params['id']);
  if (id !== null && id!== undefined) {
    this.getUser(id).subscribe(user => this.user = user);
  }
}

would instead (at least for Angular) be better written as

initialize() {
  this.user$ = this.appParameters.pipe(
    switchMap(params => {
      const id = params['id'];
      if (id !== null && id !== undefined) {
        return this.getUser(id);
      }
    }),
  );
}

and then you can do user$ | async in the template.

[cartant]

Thanks. This is an oversight - and, therefore, a bug - the rule should definitely flag these situations as failures.

[aboyton]

It might be nice to have this as a seperate rule, or at least being able to configure the options for read and write separately. I think I can see a use case for say reading properties but not writing to them, or maybe I should just be cleaning up my code to do neither. That said, being able to turn them on separately might allow easier migration.

[cartant]

Hmm, I'm not sure it's necessary. I think writing them is far worse than reading them. The use case you mentioned was pretty much the reason I wrote the rule in the first place. Why I don't have tests that specifically address that, IDK, but people copying data into class state is a pet peeve of mine.

[cartant]

I'll have a look at this, later. I have no problem changing my mind if I've remembered things incorrectly or if adding options turns out to look worthwhile, but, ATM, I'm not inclined to add options to this rule.

[cartant]

It's definitely a bug. Assignments should also effect a failure. There are tests for assignment to an out-of-scope variable:

rxjs-tslint-rules/test/v6/fixtures/no-unsafe-scope/default/fixture.ts.lint

Lines 16 to 19 in cdedcc0

	of(1).pipe(map(value => outer = value)).subscribe();
	~~~~~ [no-unsafe-scope]
	of(1).pipe(map(function (value) { return outer = value; })).subscribe();
	~~~~~ [no-unsafe-scope]

I just didn't include an assignment to a property. Doing so should effect a failure, by default.

[cartant]

An allowSubscribe option has been added to the rule in 4.24.0. The option defaults to true - the rule's previous behaviour.

The problem wasn't the assignment. It was that callbacks within subscribe were ignored.