Computed triggering the creation of an observable map invariant failed

[mattruby]

I'm having issues with computeds and observable map creation.
I have a very simple test case here: https://jsfiddle.net/7e6Ltscr/

I get the following error:

VM103:59 Uncaught Error: [mobx] Invariant failed: Computed values or transformers should not invoke actions or trigger other side effects
    at mt (mobx.umd.min.js:2)
    at B (mobx.umd.min.js:2)
    at P (mobx.umd.min.js:2)
    at V (mobx.umd.min.js:2)
    at e.merge (mobx.umd.min.js:3)
    at mobx.umd.min.js:3
    at H (mobx.umd.min.js:2)
    at new e (mobx.umd.min.js:3)
    at Function.e.map (mobx.umd.min.js:2)
    at form (VM103:59)

[mattruby]

Ahhhhhh, snap. My example works with Mobx 2.7.0.
Working version with asMap https://jsfiddle.net/cpwze6yy/

[mweststrate]

Hmm yes this is caused by observableMap with initial values causing the merge action of map to update the items. I'm seriously doubting whether this check is not too strict. There are quite some issues where people try to instantiate a Model object. Which fails if the constructor happens to contain some logic that updates an existing field....

[mweststrate]

N.B. I think the following might work around this issue:

mobx.extras.allowStateChanges(true, () => { stuff })

[mattruby]

I've tried working your suggestion into my fiddle every which way I can think of and it's still throwing.

[mattruby]

Digging into the source (Yeah, I know Ruby and Typescript? What has the world come to!) It looks like you're setting allowStateChanges for me. I'm still working my way through how that works and why the second run doesn't.

[mweststrate]

Yeah now that i think of it, i don't think it will work this way. But i hope the charm of TS gets you in the mean time 😊 Op di 24 jan. 2017 23:27 schreef Matt Ruby <[email protected]>:

…

Digging into the source (Yeah, I know Ruby and Typescript? What has the world come to!) It looks like you're setting allowStateChanges for me. I'm still working my way through how that works and why the second run doesn't. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#798 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhKrJRQrAF3ZGeURPduRhwomTRIx7ks5rVnrdgaJpZM4LsfQz> .

[mattruby]

O, I'm enjoying myself. The rabbit hole is both deep and wide over here! Anyhow, would wrapping the:
invariant(!isComputedValue(globalState.trackingDerivation),... in action.ts startAction with a check for !globalState.allowStateChanges do the trick? I'm sure I'm opening up a pandora's box full of invariant issues!

function startAction(actionName: string, fn: Function, scope: any, args?: IArguments): IActionRunInfo {
	if(!globalState.allowStateChanges) {
		// actions should not be called from computeds. check only works if the computed is actively observed, but that is fine enough as heuristic
		invariant(!isComputedValue(globalState.trackingDerivation), "Computed values or transformers should not invoke actions or trigger other side effects");
	}

[mattruby]

OK, I liked my startAction funny business above, but I'm worried about causing more problems. I'm my branch, I've changed how the map's merge functions on init. In addition, I've added a test case for this issue. I may be waaaaaayyyyy off, please let me know if I should be digging elsewhere.

[mattruby]

I think I'm way off... I'm going to hit the same issue with set in this case.

[mweststrate]

Could you try: [email protected] :)

[mattruby]

That does fix the issue. And I see that you've worked my test case in already. However, my real use case runs much much deeper than the example where I'm creating my form handlers based on those computeds. As those handlers are built they will use map.set(). I'm still working on isolating where my usage is breaking down. As set works in most cases. I have everything working with a few autoruns.
Thanks for all your help on this one! I'll keep working on isolating my issue.

[mattruby]

@mweststrate Crazy! I just isolated my issue. Please see my fiddle here for a very simple example: https://jsfiddle.net/7tfswdxt/1/

I'm overwriting or wrapping an existing map entry before returning the object. Mobx is throwing:

VM428 mobx.umd.js:2722 Uncaught Error: [mobx] Invariant failed: It is not allowed to change the state when a computed value or transformer is being evaluated. Use 'autorun' to create reactive functions with side-effects. Tried to change: [email protected]
    at invariant (VM428 mobx.umd.js:2722)
    at fail (VM428 mobx.umd.js:2717)
    at checkIfStateModificationsAreAllowed (VM428 mobx.umd.js:983)
    at ObservableValue.prepareNewValue (VM428 mobx.umd.js:2457)
    at ObservableMap._updateValue (VM428 mobx.umd.js:2101)
    at ObservableMap.set (VM428 mobx.umd.js:2043)
    at form (VM141:66)
    at Object.<anonymous> (VM141:78)
    at trackDerivedFunction (VM428 mobx.umd.js:1000)
    at ComputedValue.computeValue (VM428 mobx.umd.js:870)

PS: I'll create a new unit test on your branch!

[mweststrate]

Wrapping form in action will fix that, will add that to the error message

[mweststrate]

But that is probably vague, let's see if we can do without.. (if not in strict mode)

[mweststrate]

See also #563

Proposal to change the state modification guard to the following behavior:

Changing a piece of state is only allowed if any of the following conditions are met:

• (!strict mode || in action) && (!in computed || no observers) (supports return new observables from a derivation
• observable created in same tick && no observers (support creating and initializing observables in constructors, without marking constructor as @action (which is not possible)

For reference, the current behavior of the guard is: (!strict mode || in action) && (!in computed)

[mattruby]

I think your proposal will work well! Let me know if I can help in any way.

…

On Wed, Feb 1, 2017 at 5:50 AM, Michel Weststrate ***@***.***> wrote: See also #563 <#563> Proposal to change the state modification guard to the following behavior: Changing a piece of state is only allowed if any of the following conditions are met: - (!strict mode || in action) && (!in computed || no observers) (supports return *new* observables from a derivation - observable created in same tick && no observers (support creating and initializing observables in constructors, without marking constructor as @action (which is not possible) For reference, the current behavior of the guard is: (!strict mode || in action) && (!in computed) — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#798 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIrcj90DAp78CcZSs4cSkp3IZKDMDl8ks5rYHFqgaJpZM4LsfQz> .

-- -Matt Ruby- [email protected]

[urugator]

(!strict mode || in action) && (!in computed || no observers) (supports return new observables from a derivation)

Just to make things clear - please correct me if I am wrong:
Returning new observables from derivations (computed) is already supported... I do this all the time (with 2.x):

@computed
get user() {
  return new ObservableUser({ cart: this.cart });
}

There are two inconveniences though - the constructor can't modify observables (even outside of strict mode) and can't invoke any actions (which is kind of the same thing).
As a consequence all initialization logic must be done before the observables are introduced and actions can't be invoked as initializers from constructor or from computed itself.

So the guard is not about returning new observables, but it allows mutating unobserved state from derivation (the action si required in strict mode, but has no practical use in this case).
Shouldn't the computed allow to mutate only an unobserved state created inside the computed, similary to constructors?

However I am not sure if this should be a fix for this particular issue - imho the ObservableMap simply shoudn't call any actions (modify state) during it's initialization in the first place.

[mweststrate]

@urugator your observations are largely correct. So there are some things that could be improved, and all relate to each other

1. Computeds cannot modify state, but they can create new state. However as part of the initialization code of a new class for example, the new state might actually be modified. This is cumbersome to work around and feels artificial. This is the direct caues for this issue original, and as such could easily be fixed for map
2. but the same apply for userland class where it could not be fixed as easily
3. as mentioned, also invoken a utility action from a constructor won't help; this works as work around for modify state in a constructor function, while in strict mode. But it doesn't help when combining this with computed.

So the proposal is roughly

1. Allow actions to be invoked from computed props
2. Allow state to be changed in computed, as long as that state is 'fresh' and unobserved
3. Allow state to be changed outside of actions, even in strict mode, as long as that state is 'fresh' and unobserved (assuming that the primary value of strict mode is not triggering reactions etc at unexpected places)

[mweststrate]

Ok, I think the current solution is overly complicated, both from technical perspective and to explain. I think it could simply work by changing the semantics to:

• unobserved state can always be modified
• observed state can be modified from inside actions, or always if not in strict mode. But: not from inside computed values

It basically means strict mode is relaxed to only protect observed state. This avoids jumping through hoops when creating and initializing new state as described in this issue and #563.

[mweststrate]

Changes can be tested with [email protected]

[mattruby]

Looks good to me!

[mweststrate]

Released as 3.1.0 :)