Add List.insert and upgrade Immutable.js #29
Conversation
This commit upgrades Immutable to 3.8.1, and sets it to `>=` in `package.json`. It also adds a `List.insert()` method to typed-immutable lists. FIxes #20.
| @@ -130,7 +130,7 @@ class TypeInferedList extends BaseImmutableList { | |||
| } | |||
|
|
|||
| get(index, notSetValue) { | |||
| return this[$store] ? this[$store].get(index, notSetValue) : | |||
| return this[$store] ? this[$store].get(parseInt(index), notSetValue) : | |||
There was a problem hiding this comment.
Isn't the point of this library to not automatically type convert? Should it error here?
There was a problem hiding this comment.
So, apparently Immutable.List auto-converts strings to ints on List.get, which I did not know (and I thought was really strange). So, List.of('a', 'b', 'c', 'd').get('1') would return 'b' rather than undefined.
However, Immutable.js 3.8.1 has a bug in it where it does not auto-convert negative number strings (e.g. '-3'), only positive number ones, while 3.7.0 converted both. That was making two of the tests fail.
I could be talked into just removing that test (it is test/list.js, line 426-434) and just letting the Immutable behavior stand. It is not clear to me why this test is even there, since it did not seem to be testing anything in typed-immutable at all.
Without the parseInt, the assertion on line 431 passes, but the assertions on 432 and 433 fail.
There was a problem hiding this comment.
Oh my. This one's strange, we're either working around a bug or undefined behavior.
I think we should error if you try to list.get('1a2b3c'), but I'm also ok with merging as it is. @udnisap @davecoates any opinions?
There was a problem hiding this comment.
I guess my feeling is, when in doubt, keep the behavior we had already. If this was a from-scratch thing, I would want it to be an error.
There was a problem hiding this comment.
We should keep it as it was and match Immutable.js - this is actually an intended change, see:
immutable-js/immutable-js@7acd81b
The original test cases were based on the immutable.js test cases
There was a problem hiding this comment.
If immutable has that behaviour I think we should go with it.
|
I think we have consensus, but leaving this for a day or two, just in case. |
This commit upgrades Immutable to 3.8.1, and sets it to
>=inpackage.json.It also adds a
List.insert()method to typed-immutable lists.FIxes #20.