Change of behavior in ObservableMap.replace when using intercept in Mobx v4

[Lmzd]

Version

Mobx: 4.13.1
Node: v13.2.0

Issue

Only in mobx4

Following the changes in #1980 and #2003, visible in v4.13.1. We are facing a difference in the replace method of ObservableMap in v4.13.0.

When we intercept a change on a given property of the Map and cancel it (return null), the value is not set in the Map (expected) but the internal keys array of the Map is updated with the name of the intercepted property.

Change the version of mobx to 4.13.0 to see the previous behavior
Code to reproduce

[urugator]

Could you try this version?
EDIT: ah nevermind that won't do the trick

Notes (to self):
V5 is most likely affected as well.
Check if there are other operations that delegates to this.set/this.delete
Test also a removal of a key.
Provide internal _set/_delete with outcome rather than re-checking keys of _data.

[GLabat]

Hi thanks for your reply.

We checked MobX 5 and the code is the same as in v4.13.0, so before the change on replace.

The thing is that the internal keys of the map does not take into account a possible cancellation during interception. So we have to somehow find a way to call the setter of the given prop which triggers the interceptor handler. Depending on whether the change has to be performed or not, the keys should be updated accordingly.

[urugator]

@FredyC It seems that a fix of #1980 for V5 isn't present in the master. Do you have a clue why? (only V4 is fixed)
EDIT Is this: #2058 (comment) where it ended? Or is the PR somewhere?

[mweststrate]

@GLabat / @Lmzd would you guys be interested in making a PR? Even a unit test to protect against regressions would be super useful, as apparently we have one :)

[Lmzd]

@mweststrate Yes sure

[danielkcz]

It seems things got somehow messy in merging.

The PRs were #2057 (V5) and #2058 (V4). But then #2057 (comment) happened and V5 were being reverted and I am lost there if the issue was somehow resolved or not 🤷‍♂

[urugator]

Considering we want to respect the order of the keys in the replacement map:
If I cancel key deletition via interceptor, what is the expected position of the (non-deleted) key in the final key array?

[mweststrate]

intuitively: the same?

…

On Fri, Jan 10, 2020 at 9:23 AM urugator ***@***.***> wrote: Considering we want to respect the order of the keys in remplacement map: If I cancel key deletition via interceptor, what is the expected position of the (non-deleted) key in the final key array? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2253?email_source=notifications&email_token=AAN4NBGD7MLFWFCODV5EAGDQ5A5BRA5CNFSM4KEWJAJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEITHQKQ#issuecomment-572946474>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBAQHH5FB2EPSOPOF43Q5A5BRANCNFSM4KEWJAJA> .

[urugator]

Like this?
[1,2,3] keep 2 [5,6,7] => [5,2,6,7] ?
[1,2,3] keep 3 [5] => [5,3] ? (original 3 is out of bounds)

[mweststrate]

I'd expect 2,5,6,7 and 3,5? As it is insertion order, and the other items are added later? (if I read your message correctly) /me regrets introducing the concept of interceptors

…

On Fri, Jan 10, 2020 at 10:23 AM urugator ***@***.***> wrote: Like this? [1,2,3] keep 2 [5,6,7] => [5,2,6,7] ? [1,2,3] keep 3 [5] => [5,3] ? (original 3 is out of bounds) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2253?email_source=notifications&email_token=AAN4NBFWXEPDZL6R5AIOVOLQ5BEBRA5CNFSM4KEWJAJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEITO7LY#issuecomment-572977071>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBHGIBEP5LBFXFETJYTQ5BEBRANCNFSM4KEWJAJA> .

[urugator]

So that would be at the beginning of the key array preserving the original (non-deleted) key order...

Similar situation with set ... if I cancel set the key should also stay at the beginning of the key array, rather than respecting it's new position:
[1,2,3] (cancel set 1) [3,2,1] => [1,3,2]
Correct?

In summary whatever is cancelled should appear at the beginning of the key array in the original key order...

EDIT: Just for clarification, it's:
[original keys] (interception) [replacement keys] => [resulting keys]

[mweststrate]

yes, in other words, all original ordering is preserved, regardless the ordering in the updates. Only for items that were not present already, their relative order in the updates is relevant. (So the only effective way to reorder is to separately first remove items, and then re-add them in the desired order, otherwise all existing items stay where they are).

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs or questions.