accept plain JS Map and Set in fromJS #1858
Conversation
|
ping @conartist6 as you commented on #1723 |
There was a problem hiding this comment.
This doesn't handle WeakSet and WeakMap, and other collections.
Another way you might handle determining which kind of Seq to use is to look at the iterator methods for Iterable collections (all modern JS collections). eg if value[ITERATOR_SYMBOL] === value.entries then it's a Keyed collection. If value[ITERATOR_SYMBOL] === value.keys it's a Set collection. Otherwise probably safe to assume anything iterable is an Indexed collection (though it's often the case that value[ITERATOR_SYMBOL] === value.values - also careful since that's also true for Set)
| @@ -1,27 +1,41 @@ | |||
| import { is, List, Map, Range, Record, Seq } from 'immutable'; | |||
| import { is, List, Map as ImmutableMap, Range, Record, Seq } from 'immutable'; | |||
There was a problem hiding this comment.
Instead of having to convert this whole test file to use an alias, how do you feel about just making a new test file for Map to IMap?
There was a problem hiding this comment.
I did not knew how to call global constructor, I think the best way is to call global.Map as you did in another PR
| } | ||
|
|
||
| if (value instanceof Set) { | ||
| return ImmutableSet; |
There was a problem hiding this comment.
This should be determining a kind of Seq to use
| return ImmutableSet; | |
| return SetSeq; |
| return IndexedSeq; | ||
| } | ||
|
|
||
| if (value instanceof Set) { |
There was a problem hiding this comment.
To disambiguate, you might want to specifically reference the global Set class
| if (value instanceof Set) { | |
| if (value instanceof global.Set) { |
| if (ImmutableSet.isSet(v)) { | ||
| return v; | ||
| } | ||
|
|
||
| return isKeyed(v) ? v.toMap() : v.toList(); |
There was a problem hiding this comment.
| if (ImmutableSet.isSet(v)) { | |
| return v; | |
| } | |
| return isKeyed(v) ? v.toMap() : v.toList(); | |
| return isKeyed(v) ? v.toMap() : isSet(v) ? v.toSet() : v.toList(); |
|
Trying out an alternative approach in #1865 that has a bit of a broader scope, curious for your thoughts |
|
Your implementation is way better, and more generic. Closing in favor of #1865 |
Fixes #1723