-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Strange edge case with Map.update(...) #1984
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
It seems a tricky one, because I think that the default value happen "after" the update function. Without looking at other immutable functions, I would say that the default value should be injected in the argument of the callback if the key doesn't exist (if your are willing to open a PR, that will be great!) fix for your caseI think that in your particular case, you can handle that in your function directly: prevScore => (prevScore + score)?? 0 Or something like that (maybe |
I'm not working on that code anymore and not consuming immutable-js anywhere else so I'm less invested in the outcome of this. I will therefore unfortunately most likely not work on a PR addressing this in the near future. IIRC this line |
OK ne problem. Thanks for the report
…-------- Message d'origine --------
Le 20/04/2024 18:43, arnfaldur a écrit :
I'm not working on that code anymore and not consuming immutable-js anywhere else so I'm less invested in the outcome of this. I will therefore unfortunately most likely not work on a PR addressing this in the near future.
IIRC this line .update(player, null, previousScore => previousScore + score) worked for my original usecase.
—
Reply to this email directly, [view it on GitHub](#1984 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AAKVNRIUHOLVX5HB6UNWY53Y6KLKPAVCNFSM6AAAAABDJEV2MCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRXG4ZDKNZTGU).
You are receiving this because you commented.Message ID: ***@***.***>
|
This was discussed in this issue #1061 but does not seem to be fixed and I could not find any mentions of it in the documentation.
The issue is the behavior enforced in this test https://github.com/immutable-js/immutable-js/blame/d7664bf9d3539da8ea095f2ed08bbe1cd0d46071/__tests__/updateIn.ts#L69
Here is the test in question to save you a click.
As the test suggests, calling
map.update("field", defaultValue, id => id)
does not set the map's field todefaultValue
as I would have expected. This behavior can lead to surprising bugs like the one illustrated by the following example:This takes a set of players and scores and aggregates the score data for each player and I would expect this:
but the actual result is this:
This happens because
previousScore => previousScore + score
is the identity function ifscore === 0
andpreviousScore
is a number. To mitigate this something like this works:Less generally one can do
.update(player, null, previousScore => previousScore + score)
which works becausenull + 0
evaluates to0
whilenull != 0
. or more generally.update(player, null, (prev) => (prev === null ? 0 : prev) + score)
which should work for a larger variety of types..updateIn
has the same issue.What do you think?
Would you be open to a PR changing this behavior?
The text was updated successfully, but these errors were encountered: