Fix #3728: observables initialization should not update state version

[urugator]

• fix: [Bug] Error: Maximum update depth exceeded #3728: Observable initialization updates state version.
• fix: Observable set initialization violates enforceActions: "always".
• fix: Changing keys of observable object does not respect enforceActions.
• fix: isolateGlobalState: true makes observer stop to react to store changes #3734: isolateGlobalState: true makes observer stop to react to store changes

[changeset-bot]

🦋 Changeset detected

Latest commit: 6e81e91

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
mobx-react-lite	Patch
mobx	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[urugator]

The plan is to change these into suppressReportChangedStart/End and probably move it to initObservableStart/End (together with unstrackedStart/End and allowStateChangesStart/End as noted in comments).

[mweststrate]

yeah, some dedicated utility sounds great here to avoid copy / paste mistakes.

[urugator]

When initializing observable structures, we should do 3 things:

1. allowStateChanges so we don't violate enforceActions.
2. untracked so we don't accidentaly subscribe to anything observable accessed during init in case the observable is created inside derivation.
3. suppressReportChanged mainly to prevent version update, but also there can't be any observers yet, so there is nothing to report to, therefore we can skip the logic.

Do you agree? @mweststrate

[urugator]

I have yet to investigate failing tests. From a quick first look there seems to be a reduction in render counts, which may actually be correct. I will look into it later, once you confirm that the general direction is fine.

[mweststrate]

This feels like a potentially dangerous check, that silently might break reactivity. E.g. do the following cases behave correctly?

const x = observable({ a: 1 })
reaction(x => x.a, a => console.log("a", a))
extendObservable(x, { a: 2, b: 1 })

const x = observable({ a: 1 }, { proxy: true })
// react to new keys
reaction(x => Object.keys(x.a), a => console.log("a", a))

x.b = 1

I'm wondering if an alternative approach would be: only increase stateVersion if there are subscribers to the current atom?

[urugator]

This feels like a potentially dangerous check, that silently might break reactivity

Good point. Imo the play here is to make sure it's suppressed only on creation and not on mutation like in this case.
Key changes should invoke reactions and therefore update state version, these two go hand in hand, a reaction must always see a new state version. Therefore you shouldn't extend observable during render.
Just for clarification: extendObservable does notify about key changes.

only increase stateVersion if there are subscribers to the current atom

Purely from semantic perspective, two different states should aways have different versions, doesn't matter if someone was observing some part of the state or not. That's quite easy to reason about.
Practically speaking, I don't know. I have a bad feeling about it, but I have a hard time to come up with a convincing example. My worry is, that you change some observable x, that nobody is observing, so version stays the same. Then something unrelated to mobx - props/state or other external sync store - changes in such a way, that the next render would read from x. Now if react would check whether mobx external state changed, it doesn't know about the change of x, even though it's about to use it together with rest of the world (other observables, state, props, other external state) to render. I am not sure if that alone is a problem. The goal here is to avoid inconsistencies and to have an inconsistency, the x would probably have to be somehow correlated with the rest of the world (props/state/other store). But then, if they're correlated, they had to change at the same moment and therefore something, somewhere had to already signal a new version? It's hard for me to contemplate about this, especially when I don't know what react exactly does under the hood and what it's assumptions are. If version always changes, I don't have to think about this...

[mweststrate]

Purely from semantic perspective, two different states should aways have different versions, doesn't matter if someone was observing some part of the state or not. That's quite easy to reason about

If version always changes, I don't have to think about this...

Agreed, I think these principles weighs stronger than my concerns. Let's continue this direction.

[urugator]

There is a fundamental problem with subclasses - new Subclass() always counts as a state update, because the subclass's constructor calls makeObservable on an already observable object (this).
Strictly speaking it's correct behavior, because constructors can insert any arbitrary logic between individual makeObservable calls and there is no guarantee new Subclass is called from transaction, so a reaction invoked in between these makeObservable calls should see new state version.

What we would need is to treat all modifications of the same atom inside the same batch it was created as part of initialization. To do that we would need to introduce globalState.batchId, assign this batchId to every atom when created and suppress reportChanged when atom.batchId === globalState.batchId. Now you don't have to manually demark initialization sections - they span from atom creation until the end of the current batch. If you call new Subclass() from action as a good citizen, the state version stays same, no matter how many times you called makeObservable. Thoughts?

[mweststrate]

Ok, that sounds smart! It might serve in the future as well to better handle any weird edge cases around reaction creations in actions, recursive updates etc etc, although I believe we handle those all consistently now.

[mweststrate]

Really awesome work @urugator, great fixes and clear to follow! Probably good to run the perf tests. Left two comments, but beyond that looks fine to merge by me!

[mweststrate]

The initObservable method now lifts all initializers outside the constructor body as direct statements, so it might be needed to initialize every field now to undefined here at property declaration, to make sure we don't miss inner class optimization that takes place when V8 scans the constructor body for unconditional field assignments (sorry, can't find the authoritive source on that anymore, so might be outdated, but probably doesn't harm)

[urugator]

Will take a look at that. I wonder whether "useDefineForClassFields": true has any effect on this, since it replaces assigments with the Object.defineProperty calls.

[urugator]

Actually

class Foo {
 prop!;
}
// transpiles to
"use strict";
class Foo {
    constructor() {
        Object.defineProperty(this, "prop", {
            enumerable: true,
            configurable: true,
            writable: true,
            value: void 0
        });
    }
}

So I quess it doesn't matter?

[mweststrate]

Yes you're right, I still have 7 year old micro-optimizations against ES5 in my head when I wrote first versions 😅. Feel free to merge!

[Egor-Chib]

We are using MOBX 6.10.2
and it looks like the issue persists: if I create children component, which do "new Something()" and "something" has in constructor "makeObservable" - this will cause re-render of parent.