ramda / ramda

:ram: Practical functional Javascript
https://ramdajs.com
MIT License
23.8k stars 1.43k forks source link

`dissocPath` converts Object to Array when number in path #2543

Open ghost opened 6 years ago

ghost commented 6 years ago

In dissocPath targeting an Object which have numeric keys, with path having segment for the object typeof === 'number' causes the Object being converted to an empty Array.

Here is example in REPL https://goo.gl/5atUpi

Here is proof from debugger ran in Node 9.3.0 where input is definition, output is definitionWithoutTile and path is ['content', 3]. You can see, that in output is na empty Array.

https://www.evernote.com/l/AkMyGtHNf19EZJlDZw1U8Cq5e_yfDNXb08k

And here is the same code, where number 3 in path is converted to String and output is dissociated Object as expected.

https://www.evernote.com/l/AkMMFnPjf3RAGoKACpSZpDzOFlmTNGuhEC4

CrossEye commented 6 years ago

It's possible that there could be a fix for this for dissocPath. I'm not quite sure what that would be, but a PR would be welcome.

However, for assoc, this is clearly working as expected.

Given

const tree = {2: {v: 1}, a: [{b: 'hey'}]}

You point out the difference between these:

assocPath(['a', 1], 'hi', tree)
//=> {2: {v: 1}, a: [{b: 'hey'}, 'hi']}

assocPath(['a', '1'], 'hi', tree)
//=> {2: {v: 1}, a: {0: {b: 'hey'}, 1: 'hi'}}

That is by design. When originally written, assoc and assocPath worked only with objects, and had no idea about arrays. We changed it so that when passing integers as keys, they would act as array indices. This is a much more useful API, allowing us to work on exactly the kinds of mixed data presented in tree.

That allows for these sorts of behavior:

assocPath(['a', 1], {b: 'howdy'}, tree)
//=> {2: {v: 1}, a: [{b: 'hey'}, {b: 'howdy'}]}

assocPath(['c', 0], 'howdy', tree)
//=> {2: {v: 1}, a: [{b: 'hey'}], c: ['howdy']}

assocPath(['a', 1], 'aloha', {a: ['hey', 'howdy', 'hi'], 2: {v: 1}})
//=> {2: {v: 1}, a: ['hey', 'aloha', 'hi']}

With these changes, for assoc and its companions, integers are array indices and strings are object keys, even such strings as '0', '1', or '2'. Ramda simply assumes the user is passing the correct type. But it might make sense to alter this a bit for dissoc, using the type of the object whose property we're removing. As I said, a PR would be welcome.

MilanLempera commented 6 years ago

Hi @CrossEye, I am ok with assocPath behavior, but dissocPath now destroys data, because under certain circumstances calls functions for arrays (update and remove) on object.

I think this can be solution https://github.com/ramda/ramda/pull/2545

polytypic commented 6 years ago

With these changes, for assoc and its companions, integers are array indices and strings are object keys, even such strings as '0', '1', or '2'. Ramda simply assumes the user is passing the correct type.

That is reasonable.

Here is a thought exercise:

Independently from an operation on paths while, of course, having enough information that it specifies the types of compatible data structures.

But it might make sense to alter this a bit for dissoc, using the type of the object whose property we're removing.

IMHO, in general, if you have a bunch of functions operating on some abstraction, like "paths" in this case, then all of those functions should treat those things the same way. That way it is simpler and easier to understand all of the combinations, because there are fewer of them.

So, I would make a choice here. Either all functions operating on paths allow mixed handling of integer keys or none of them do.

If you allow mixed handling of integer keys then you arguably should not allow assoc to build non-existent parts of the data structure, because then you have to make an arbitrary choice (between building an array or an object) in assoc and whichever way you choose is likely to be wrong in some cases where a user is not distinguishing between integer keys and numeric strings.

...

In my Partial Lenses library I have specifically chosen to allow lenses to construct non-existent parts of data structures. In Partial Lenses non-negative integers are treated as lenses that construct arrays and strings are treated as lenses that construct (plain) objects. However, because lenses are functions, users can implement lenses that can perform mixed handling of objects/arrays and can also make a specific choice on what to do in case the target is neither an array nor an object. Here is an example:

const index = (i, otherwise = i) => L.cond(
  [R.is(Array), i],
  [R.is(Object), i.toString()],
  [otherwise]
)

The above function, which is supposed to be given a non-negative integer, constructs a lens that creates arrays, when targeting arrays, and objects, when targeting objects. In case the target is neither, the otherwise lens is used instead, which defaults to building an array. Here is the above example in a live playground with some usages.

CrossEye commented 6 years ago

@polytypic:

This is a very interesting comment, and it's going to take me a bit of time to absorb it. I'm not sure how much I agree or disagree immediately.

One objection is that the path is not the only value that we work with here, and how much would we want to respect the integrity of the object serving as the base of the transformation? The fact that a node is an Array versus an Object is fairly fundamental, and changing that is also problematic.

What I'd really love to see is whether we can determine some useful laws about functions like this. We probably could manage

// ∀ path, val, obj
R.equal(obj, R.dissocPath(path, R.assocPath(path, val, obj))) //=> true

But the reverse might be harder:

// ∀ path, val, obj
/R.equal(obj, assocPath(path, R.path(path, obj), R.dissocPath(path, obj))) //=> true

because of non-existent paths and undefineds.

I will spend some time thinking about this.