Strange edge case with Map.update(...)

[arnfaldur]

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?

[jdeniau]

It seems a tricky one, because I think that the default value happen "after" the update function.
It is useful if the expected value is to return the default value if the function return undefined.

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 case

I think that in your particular case, you can handle that in your function directly:

prevScore => (prevScore + score)?? 0

Or something like that (maybe (prevScore?? 0) + score, I do not have a computer now to validate).

[arnfaldur]

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.

[jdeniau]

OK ne problem. Thanks for the report Julien Deniau

…

-------- 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: ***@***.***>