[WIP]: Change of behavior in ObservableMap.replace #2255
Conversation
…ow_mobx4 Update typings to allow flow to take async generators - async function* - MobX 4
Mobx4 master
* decorate - compose decorators for a single prop - Reduce multiple decorators to a single decorator - Added a test for multiple decorators (@action + custom) on a function property - Added a test for multiple decorators (@observable + @serializable) on a regular property - Added a usage example in readme under `decorate` section * delete package-lock * doc: update README - Change caveat note about docorators composition as suggested in mobxjs#1652 (comment) - Add decorators application order * Update README.md
…orator-mobx4 decorate - compose decorators for a single prop (mobx4)
Same issue mobxjs#1448, new case.
Decorate: Allow modifier when strict (MobX 4)
…side a computed value (mobxjs#1706) * make allowStateChanges allow changes inside computed value in order to fix MST mobxjs/mobx-state-tree#967 * added allowStateChangeInsideComputed * added unit test * small unit test error * safer implementation * Alternative solution * Revert "Alternative solution" This reverts commit e15bc78. * improve unit test * Changed to more predictable restoration
|
I think .replace is different from the others, in that sense, that it
should act I think as a full clear + all new items chained, rather than as
one operation. So this always results in the insertion order of the new
data (which was what that issue was about IIRC), and intercepting a single
key beying re-added would effectively result in the loss of that key in the
map?
Maybe the best way to tackle this is by starting a PR with (.skip-ed) unit
tests so that we have a cohesive overview of all the cases and past issues?
(I noticed I tend to scanning comments to quickly and sometimes
misunderstand them, where unit tests leave little room for interpretation)
…On Sun, Jan 12, 2020 at 10:56 PM urugator ***@***.***> wrote:
That would mean to reintroduce #1980
<#1980>, in which case I am not sure
about correctness of the current impl.
I also think the current PR doesn't handle correctly key change detection
for cancelled additions:
If the key isn't in original map, set gets cancelled, keyHasBeenSet is
false, keysChanged is set tu true even though, the keys are not modified.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#2255?email_source=notifications&email_token=AAN4NBCOTLJNAOG4PYCEIP3Q5ON2HA5CNFSM4KEYFGRKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIXGRYQ#issuecomment-573466850>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAN4NBATX4INUGZCM6FBYO3Q5ON2HANCNFSM4KEYFGRA>
.
|
Seems reasonable to me. { 3:3, 2:2 }
{ 1:1, 4:4, 5:5, 2:20 }@Lmzd does that work for you or your use case? For the next major I would consider using single |
|
@urugator I implement those 2 tests in Can you check if the tests have the good logics please ? |
|
Omg, I haven't realized that |
|
@Lmzd The PR just went through so you need to rebase your work onto a |
|
I think |
|
Changes on |
|
I am kinda surprised you went this way, V4 branch history is totally different, but looks like you got it sorted out, so it's fine. We will squash when merging, so it won't matter. |
|
So where are we with this? Can we move it forward somehow? The test is failing :/ A new issue #2274 for the |
|
Based on the CI output, |
|
Oh lol, was blindly assuming the changes where made to the v5 one because
that tests weren't failing, and v4 tests needed updating. Too early monday
morning, lol.
…On Mon, Feb 10, 2020 at 10:16 AM Daniel K. ***@***.***> wrote:
Based on the CI output, v4 tests are failing, v5 is fine because there
are no changes to it in this PR.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#2255?email_source=notifications&email_token=AAN4NBF5NSQTD5U6WT2NMS3RCESRLA5CNFSM4KEYFGRKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELH6O6Q#issuecomment-584050554>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAN4NBE52F7DWTK7X6W6T3TRCESRLANCNFSM4KEYFGRA>
.
|
|
To avoid double work: I think this one can be put [on hold] as the same issues will be investigated by @urugator, including tests / changes / fixes by this PR |
|
Closing in favor #2287 |
Issue #2253