Description
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.
it('identity with notSetValue is still identity', () => {
const m = Map({ a: { b: { c: 10 } } });
expect(m.updateIn(['x'], 100, id => id)).toEqual(m);
});
As the test suggests, calling map.update("field", defaultValue, id => id)
does not set the map's field to defaultValue
as I would have expected. This behavior can lead to surprising bugs like the one illustrated by the following example:
const scoreData = [
['alice', 0],
['brian', 2],
['chris', 0],
['alice', 1],
['brian', 0],
['chris', 0],
['alice', 1],
['brian', 1],
['chris', 0]
];
const scores = scoreData.reduce(
(scores, [player, score]) =>
scores.update(player, 0, (previousScore) => previousScore + score),
Immutable.Map()
);
This takes a set of players and scores and aggregates the score data for each player and I would expect this:
console.log(scores.toJS())
> { chris: 0, brian: 3, alice: 2 }
but the actual result is this:
console.log(scores.toJS())
> { brian: 3, alice: 2 }
This happens because previousScore => previousScore + score
is the identity function if score === 0
and previousScore
is a number. To mitigate this something like this works:
const scores = scoreData.reduce(
(scores, [player, score]) =>
scores
.update(player, 0, (previousScore) => previousScore + 1)
.update(player, 0, (previousScore) => previousScore - 1)
.update(player, 0, (previousScore) => previousScore + score),
Immutable.Map()
);
Less generally one can do .update(player, null, previousScore => previousScore + score)
which works because null + 0
evaluates to 0
while null != 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?