selfrefactor / rambda

Faster and smaller alternative to Ramda
https://selfrefactor.github.io/rambda
MIT License
1.65k stars 89 forks source link

[BUG]assocPath interprets string path element as integer, creating arrays instead of objects. #748

Open mmikeyy opened 1 week ago

mmikeyy commented 1 week ago

Describe the bug const obj = {}; R.assocPath(['a', '2'], value, obj) produces obj == {'a': [undefined, undefined, value]}

This is totally wrong.

With Ramda, the result is the expected: {'a': {'2': value}}.

I'm handling big complex data structures that I update with assocPath. This bug screws up a program on which I spent many hours. Now I understand why I was getting unexpected results. Hopefully this will be fixed soon. Going back to Ramda in the meantime.

Context(which version of library)

selfrefactor commented 1 week ago

Thanks for filling this issue and for the good explanation. I will write once a bug is fixed and new version is released.

selfrefactor commented 6 days ago

I will have to close this issue as the reason for this behavior is that Rambda supports dot notation for path in R.assocPath and other similar methods.

What I will do is to include this information in differences between the two libraries.

mmikeyy commented 5 days ago

Hi,

I had started writing an email about exactly this after your reply to the first bug report. I was afraid that fixing the problem I mentioned would break the dot notation. But I finally refrained from sending it, thinking you didn't need me to figure this out...

Anyway, I gave the issue some thought and came to a conclusion that differs from yours.

The problem is that with dot notation, it's impossible to know the programmer's intent with a numerical key/property.

So R.assocPath('a.2.b', value, obj) will behave differently - and correctly - depending on whether obj.a is an existing array or object, but will create an array obj.a with {b: value} at index 2 if obj.a doesn't exist.

That's reasonable. And one would expect to be able to force the creation of an object instead of an array by providing 2 as a string ['a', '2', 'b'] in the example above.

I thought that the problem perhaps is that you always convert array paths (['a', '2', 'b'] or ['a', 2, 'b']) to dot notation ('a.2.b' in both cases) as a first step so the information as to the type of 2 (numeric or string) is lost. But this is not the case since you handle assocPath(['a.b', 'c'], value, {}) properly (produces {'a.b': {c: value}}, not {a: {b: {c: value}}}.

You propose to leave things as they are and just include this in the list of differences with Ramda. I'm not sure this is the best choice. Wouldn't it be better to have a rule for ambiguous cases (e.g. numeric value in dot notation will correctly access existing arrays or objects, but will create arrays when pointing to the void), and allow the user to express his intent (['a', 2, 'b'] or 'a.2.b' to create arrays, but ['a', '2', 'b'] to create an object) and have the function behave accordingly?

Insisting on treating ['a', 2, 'b'] and ['a', '2', 'b'] identically and stating in the documentation that it's a feature, not a bug, would be tantamount to saying something like "we understand what you want and would have no problem doing it, but we just won't!". In addition to disregarding the programmer's intent for no apparent reason, the insistence on treating a string value as a number stands in contradiction with the basic rule that object properties are never numerical, and array indices can only be numerical. It's a bit surprising to be told 'I see your string, but I'll treat it as a number if you don't mind'. But I do mind!!

I hope I made my case! Perhaps I'm missing something. I too at some point thought that the dot notation was a big problem in this context. But I came to the conclusion that it isn't. The rule just needs to be known: numeric values in paths expressed in dot notation will correctly access existing arrays/objects, but will create only arrays (not a change, that's how it is now!). This rule being stated, one will naturally understand the need to use array paths instead of dot notation when numeric strings are used as object properties and the function is expected to create an object. I see no reason for shutting that door!

Luckily it is possible to import functions from Ramda one at a time. So I import this one function. I know I'll forget from time to time, unfortunately...

Thanks for your time and patience!

Michel

On 23 October 2024 14:28:01 (-04:00), Dejan Toteff wrote:

I will have to close this issue as the reason for this behavior is that Rambda supports dot notation for path in R.assocPath and other similar methods.

What I will do is to include this information in differences between the two libraries.

— Reply to this email directly, view it on GitHub https://github.com/selfrefactor/rambda/issues/748#issuecomment-2433110295 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPBGUEVI5E6M2QFRCODUATZ47TDDAVCNFSM6AAAAABQMXDEDCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMZTGEYTAMRZGU . You are receiving this because you authored the thread.

-- Sent with Vivaldi Mail. Download Vivaldi for free at vivaldi.com

selfrefactor commented 4 days ago

Thanks for the long and detailed reply.

You make a strong case, so I will make a major bump after the current in progress release and I will include a fix for this issue.

The thing is that it might be a while until that happens, as I need to plan what other major fixes I can include.