Intransparant state mutations of BehaviourSubjects is possible via map(...) and tap(...)

[CaspianVahagn]

Describe the bug

When mutating a value in map or tap by regular object and field assignment, the whole state of the Behaviour subject will change.

While it is expected that mutating a Object will directly change the object, it is intransparent, when using a Behavioursubject as a sources and passing it multiple locations as Observable.

When mapping and subscribing to an observable that is derived from a Behavioursubject, it will subsequently effect all other Observables in other places aswell that listen to the same value.

See the code example.

This can lead to dangerous and unexpected bugs in large Projects (for example with Angular) in which multiple teams do this.

Proposals are:

• Note it in the Documentation that making it a Observable will not "magically" turn it into a safely mutatable object.
• Adding a parameter to Behavioursubjects which makes the object mutable and throws erroros when subsequent manipulations happen
• when turning the Behaviousubject into an observable, add a parameter to asObservable() such as "mutable, clone, default"
  Which when "muted" will throw errors when mutated. Or be cloned from the original state when created such an observable, or on default allow any mutation

Expected behavior

Objects mutations within tap(...) or map(...) of derived obserables from Behavioursubjects should not mutate the original state of the Behavioursubject

Reproduction code

const bSubj = new BehaviorSubject({ a: 1, b: { c: 'hello', d: 'world' } }); 

const obsA = bSubj.asObservable(); // two observables
const obsB = bSubj.asObservable(); // two observables

obsA
  .pipe(
    map((obj) => {
      obj.b.d = 'Github';  // mutating the object in the observable changes the state of the Behaviour subject
      return obj;
    })
  )
  .subscribe((value) => console.log('observable A', value));
// output: observable A
// {"a":1,"b":{"c":"hello","d":"Github"}}


obsB.subscribe((value) => console.log('observable B', value));
// output: observable B
// {"a":1,"b":{"c":"hello","d":"Github"}}
// here the change of the "world" to "Github" is not expected

Reproduction URL

https://stackblitz.com/edit/rxjs-xgtmg6?file=index.ts

Version

7.8.1

Environment

NodeJS, Angular, Javascript

Additional context

No response

[sijakret]

I also noticed this.
In several angular applications i have seen problematic patters where data was mutated in place in map operators.
It can be harmless but in case the data is shared it can lead to severe headaches!

I also agree that the documentation could be clearer about this.
In case your map(or whatever operator) has side-effects (in particular mutating its input data) all sorts of alarm bells start ringing for me immediately.

I believe this can be solved to some extent in the application space using typescript:

// lifted directly from this article: https://dev.to/bwca/deep-readonly-generic-in-typescript-4b04
type DeepReadonly<T> = Readonly<{
  [K in keyof T]: 
    // Is it a primitive? Then make it readonly
    T[K] extends (number | string | symbol) ? Readonly<T[K]> 
    // Is it an array of items? Then make the array readonly and the item as well
    : T[K] extends Array<infer A> ? Readonly<Array<DeepReadonly<A>>> 
    // It is some other object, make it readonly as well
    : DeepReadonly<T[K]>;
}>

// operators on this subject should HAVE to create a copy in case they mutate the data
const subj = new BehaviorSubject<DeepReadonly<{ a: string}>>({ a: 'test' });
// line below gives: Cannot assign to 'a' because it is a read-only property.ts(2540)
subj.pipe(map(data => data.a = 'overwritten')).subscribe(...);

what do you think?

[benlesh]

Honestly, I don't think we'd want to do this, as immutability can be composed in, rather than baked in. The main reason I would want to avoid this is performance. Duplicating objects and the GC there in?

You might want to look into structuredClone as a possible way to create copies of deep objects and set values on them easily? Otherwise, I'd look into something like immer or the like.

[sijakret]

The whole point of my proposal was to show a possible solution to solve this in the application space (not in rxjs) using readonly typings :). So all good..
I don't worried about performance here.
What worries me is that semantics. The inputs to a map() operator should always never be mutable by default.
This can lead to impure operators.
Exposing Readonly data in an observable (via ts) makes errors where the input data is accidentally modified less likely. (such a modification would be shared to ALL other subscribers)