RxJS should prevent Subject values to be modified by anyone but a "next" push.

[vicmarcal]

Bug Report

Current Behavior
Subjects values can be easily messed up via getValue() or subject.value since they are returning references. Imho, by design, the only way a Subject value should be able to be modified is through Subject.next(). Otherwise the Subject.value can be modified by a function1() and hence a function2() can obtain inconsistent values.

Expected behavior
RxJS should prevent Subject.values to be modified by anone but a "next" push.

Reproduction
This reproduction example shows 2 Test Cases.

First test case is just reading the subject.value, manipulating the returned value and as a side effect modifying subject value (and probably totally unnoticed for the developer due the "sticky" references Javascript has).
Worst thing is that a second function, which could live in a different file/method, now is seeing a wrong Subject value and outputting a wrong result.

Second test case is showing how subject.getValue() is really returning also a reference, so the behavior is exactly the same. (Just checking if it was making any difference really)

RxJS should protect the value/s being stored in the Subjects. If a value was pushed to a Subject, no one should be able to modify such value. This is imho critical in a language, as Javascript is, where references are so sticky.

• Repo link:
  https://stackblitz.com/edit/rxjs-mess-subject?file=index.ts

Environment
Latest RXJS, provided in the stackblitz.

Possible Solution
If, for retrocompatibility, subject.value and getValue() are decided to keep returning references, then I would suggest to create a new getDeepValue() method which returns a deep clone to avoid the mess created in the example and heavily recommend to use getDeepValue() in documentation.

[kwonoj]

In my opinion I think this is same as #5398 (comment) and it's caller's responsibility to protect mutation.

[vicmarcal]

In my opinion I think this is same as #5398 (comment) and it's caller's responsibility to protect mutation.

a) RxJS should be protecting the pushed value to the Behavior Subject to not be changed unless you want side effects through your whole code. You can see, in my tiny example, how easy is to mess the value and how other functions using getValue can then get side-effected. This would be a big hole in the design of RxJS itself, so I don't really buy the idea this is an expected "feature".

b)However, lets believe this is the "expected" behavior and any function/method can "mess" (sorry I can't find a better word) the value stored in a Subject through reference as the repro case is showing, then RxJS should be sending to all the "subscriptions" the change done by reference so they can react accordingly.

Current implementation is not doing a) nor b).

[kwonoj]

I am personally not in agreement for both, but probably other team member might have different opinions so I'll leave this as opened. However, at least I do not think this is a known bug to be filed, so moving this as into discussions for further update.

[vicmarcal]

Then I am open to ideas. However my bet is that right now developers using "getValue()" or ".value" are probably mutating by reference the values stored in the Subjects and scratching their heads when another method returns a "mutated" value from the Subject.value.
Do really RxJava or other implementations really return a reference and not a deep copy in getValue()? Just curious.
Not protecting the Subject value from unwanted changes seems illogical from my developer perspective.

[benlesh]

We can't really do this. If you're worried about mutations, you could always use a library like Immer and next those into the subject.

The problem is, we'd have to perform deep copies of objects in order to really protect completely. Deep copying is pretty slow and very inefficient. Also then the expectation is would be doing it everywhere. Like scan, for example.

// This naive impl would not work, because we're just putting the same
// references in a new object.

const original = { a: [1, 2, 3], b: { name: 'Ben' } };

// naive "copy"
const copy = { ...original };

// we can still mutate the inner references though.
copy.a[0] = 'OOPS';
copy.b.name = 'LOL';

console.log(original.a); // ['OOPS', 2, 3]
console.log(original.b); // { name: 'LOL' }

[vicmarcal]

Thanks @benlesh for your feedback.

We all should be worried about developers mutating without noticing the inner value of a Subject, not just me. I'm just hoping RxJS maintainers are as worried as developers using this amazing library are.

If a Subject value can easily be "messed" with the basic PoC I shared, then we all (maintainers and developers) should be really worried. I am not going to raise the cheesy flag of "security concerns" but the cheesy flag of "unreliable values".

Can the Subject value be protected always? Probably no, as you correctly pointed, due performance. However, this discussion started with the "getValue()" case. So let me share some context.

During the last years, and since Angular adopted RxJS as its main library, I am reviewing and fixing issues due RxJS mishandling from different projects and companies, and the handling of "getValue()/subject.value" is creating undesired mutations in 90% of those projects. I define them as "unwanted" because after the review, the engineers, who knows about what "a reference is", couldn't believe "getValue()" was not serving them a "deep/safe" copy. Yeah, we can tell them RTFM or maybe RxJS could prevent leaking references at least in this particular case.
Some more stats: About 50-60% weren't suffering issues at that time (good luck in the future) due to undesired mutations, however, more than 40% had (funny) comments of the kind "requesting again because Subject seems to be missing the values", "Subject is unreliable, request again", "HACK!!! Request Shippings again because sometimes the Subject seems to lose them" or even incredible and complex architectures due the undesired mutations via reference. Sometimes a developer was using a Spread operator or Object.assign on the returned value, which was clearly insufficient (as you pointed) since more than a shallow copy was needed; other projects weren't applying any type of "copy" at all. That's the real picture.

It's funny(?) to see projects following the "private Subject, public Subject.asObservable()" strategy to prevent, among other things, making the ".next()" public and protecting Subject to be pushed/changed from outside of the Service, but they are returning an undesired**unnoticed reference to the Subject value from the service with a public method which "returns this.subject.getValue()".

Regarding performance. It would be amazing if RxJS just uses Deep copies but this would strike performance a lot, however what about creating the "deep copy" in the getValue() method instead of returning the reference?

Probably, due retrocompatibility subject.value and getValue() may stay as they are nowadays: just returning a reference. This way nothing is broken for old projects when updating the library (who knows if someone is abusing of mutations in a weird way?), and if someone "needs performance" they can use their own way of "copying" (Object.assign, JSON hack, re-constructing the objects, spread operators...) using them.

But maybe a new .getDeepValue() method could be created? One which returns a deep copy (lodash clone-alike) of the Subject stored value, and which is recommended to use over the other two unless you need the boost of speed and you know what you are really doing when using the original ones.

For sure RxJS would not be avoiding all the potential "mutations" paths, but at least fixing a low-hanging fruit that is affecting projects widely.

[Sawtaytoes]

I reread what you wrote twice, and I still don't quite understand.

Could you provide some code for where you'd expect to receive a deeply-copied object?