Skip to content

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

Open
@arnfaldur

Description

@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?

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions