Skip to content

Map.update is broken? #1061

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

Closed
benny-medflyt opened this issue Feb 23, 2017 · 4 comments
Closed

Map.update is broken? #1061

benny-medflyt opened this issue Feb 23, 2017 · 4 comments

Comments

@benny-medflyt
Copy link

Immutable version 3.8.1

I must be doing something seriously wrong, or have missed something fundamental:

$ node
> var Immutable = require('immutable');
> var m = Immutable.Map();
> m.size
0
> m = m.update(42, "hello", (x => x));
Map {}
> m.size
0

Why wasn't "hello" inserted into the map? According to the docs, update is:

Equivalent to: map.set(key, updater(map.get(key, notSetValue))).

Let's try running that instead:

> m = m.set(42, (x => x)(m.get(42, "hello")))
Map { 42: "hello" }
> m.size
1

That seemed to work just fine! So why didn't "update" work?

@tolmasky
Copy link
Contributor

I've investigated this a bit. The problem seems to be:

  1. On line 784, it checks whether the new value is the same as the existing value. If it is, it considers no need to do anything. In this case, the existing value is the not-set-value. So it SHOULD still do something because the not-set isn't there yet.
  2. On line 98 if no value is set, it returns undefined. It should just return itself.

tolmasky added a commit to tolmasky/immutable-js that referenced this issue Feb 26, 2017
@leebyron
Copy link
Collaborator

leebyron commented Feb 27, 2017

This is an edge case to how update works with identity functions and default values.

See documentation discussion: http://facebook.github.io/immutable-js/docs/#/Map/updateIn

If the updater function returns the same value it was called with, then no change will occur. This is still true if notSetValue is provided.

@tolmasky thanks for investigating, I added some additional tests for this case and unfortunately I think the changes you made would have broken them. Specifically, that first line you reference is handling the identity function case, and the second line - NOT_SET implies the value doesn't exist anymore, so actually notSetValue should be used there instead of this.

@benny-medflyt
Copy link
Author

If the current behavior is what is intended, then I must say it is very surprising.

I see that something is indeed mentioned under "updateIn", but the documentation for Map.update does not mention this. It should be mentioned there as well, and the line saying that update is

Equivalent to: map.set(key, updater(map.get(key, notSetValue))).

should be removed, since it's clearly not true.

Thank you

@leebyron
Copy link
Collaborator

I agree this corner isn't a great one. I'm hoping to improve it in a future major version. In the meantime these are great documentation suggestions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants