observable map modifications outside action do not rise errors with useStrict

[fedorl]

map.set(existingKey, x) in the strict mode outside an action fails as expected with Invariant failed, requiring an action context.

However, map.set(nonExistentKey, x) as well as other modifying operations e.g. map.delete(), map.clear() silently run without any error even when no action is present.

Any operation that modifies anything in the observable map state outside of an action must fail; the same way as an existing key replacement and the same way as ObservableArray.clear() or ObservableArray.push().

Current behavior is not consistent and loses the value of useStrict for maps, because one can't rely on it anymore. Consider two equivalent operations set(key, v); set(key, v) and set(key);delete(key);set(key); which have now completely different outcome, one will fail and one will work outside of an action.

Not only does it allow uncontrolled state mutations, it does it in an unpredictable way depending on other mutations and their order.

`
class PropTest {
@observable propMap = observable.map();
}

const key = 'testKey';
autorun((r) => {
const val = propTest.propMap.get(key);
console.info('------> ' + val);
});

propTest.propMap.set(key, 1);
propTest.propMap.set(key, 2); // -> Invariant failed as expected

//below runs fine which is not what one would expect in strict mode
propTest.propMap.set(key, 1);
propTest.propMap.delete(key);
propTest.propMap.set(key, 2);
propTest.propMap.clear();
`

If there are implementation difficulties, this limitation should be reflected in documentation, as this inconsistency seems to be an alarming issue.

[urugator]

I can confirm that only value modifications of existing keys throw, key modifications (introducion of new key, delete, clear) don't throw.
Here is the fiddle to check the behavior

[mweststrate]

Confirmed, this is probably caused by the fact that map uses actions internally, https://github.com/mobxjs/mobx/blob/master/src/types/observablemap.ts#L198.

Either before running the internal action, checkIfStateModificationsAreAllowed(), should be called, or runInTransaction should not start an action (but still do the batching!).

PR's are welcome :)

[roperzh]

Hey @mweststrate , sorry to bother! I've been looking at this and I wanted to ask before proceeding.

Since checkIfStateModificationsAreAllowed expects an Atom as an argument, should we instantiate a new Atom when the map is created? I'm thinking in something in the lines of how ObservableArray does it.

Thank you!

[urugator]

I may be wrong, but I think the idea is to perform the check on the map's internal observable structures whenever they are being modified in runInTransaction (which provides batching, but unfortunately supresses the strict mode).
To obtain an atom from observable you can use extras.getAtom(observable).
So for example here, the observable this._keys is being modified so we must check if it can be modified, before the transaction is invoked:

checkIfStateModificationsAreAllowed(getAtom(this._keys)) // throws in strict
runInTransaction(() => {
  this._keys.remove(key);
})

Similary here the observable entry is being modified, so the same check needs to be performed. The problem is, you still have to push the check before runInTransaction, which is outside of this nested method (and happens conditionally)...

So the second idea is to replace runInTransaction with a similar function, which doesn't call allowStateChangesStart and allowStateChangesEnd, but still calls startBatch and endBatch. (it may be a bit more complicated, dunno)
Which seems useful for cases like this and also like a better/cleaner solution (there may be some performance benefits too).

[mweststrate]

The issue is I think simply that the methods on observable map are marked as @action, where runInTransaction should be used. The main thing withholding fixing this bug is that it will probably be a breaking change for a lot of people if they modified a map in some code that is not an action yet, that worked so far, and not anymore after fixing this.

[smikula]

I see this is fixed -- any plans for when a release including this fix will come out?

I'm glad to see that transaction is sticking around. It's really helpful in my project to be able to distinguish a transaction from an operation that can modify state.

[mweststrate]

Just released as 3.3.1 😊

…

On di 3 okt. 2017 01:23 Scott Mikula ***@***.***> wrote: I see this is fixed -- any plans for when a release including this fix will come out? I'm glad to see that transaction is sticking around. It's really helpful in my project <https://github.com/microsoft/satcheljs> to be able to distinguish a transaction from an operation that can modify state. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#940 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhLLYeASEHt9JumSV6mnwCOAOXtfrks5soXBrgaJpZM4M4R1Y> .