omniscientjs / immstruct

Immutable data structures with history for top-to-bottom properties in component based libraries like React. Based on Immutable.js
374 stars 21 forks source link

structure.cursor, structure.reference, and reference.cursor do not properly handle valid but falsey keys #50

Closed jabjab closed 9 years ago

jabjab commented 9 years ago

Immutable's cursors, including Cursor.from, use valToKeyPath under the hood to convert '' to [''], but structures and references handle .cursor(''), as well as .cursor(0) and other falsey keys, as though they are all .cursor() (and structure.reference('') is handled as structure.reference()). Because of this, the following happens:

> var s = immstruct({'': 3, a: {'': 5}})
> s.cursor().cursor('').deref()
3
> s.cursor('').deref()
Immutable.Map({'': 3, a: {'': 5}}) // Prettified for brevity
> var r = s.reference('a')
> r.cursor().cursor('').deref()
5
> r.cursor('').deref()
Immutable.Map({'': 5})

While an edge case, this means users who can't enforce a policy restricting keys to values that do not evaluate to false have to use the same sort of checks valToKeyPath does already to ensure any supplied values are properly converted to key paths.

dashed commented 9 years ago

Agreed. This should be fixed. key path conversion should match exactly how Immutable Cursors does it: https://github.com/facebook/immutable-js/blob/3799d5f223315a41ceb524b627e558c548f3b7c5/contrib/cursor/index.js#L305-L309

This can be patched at the following lines:

mikaelbr commented 9 years ago

Very true. Good catch. Fixed in cab0d24.