radash icon indicating copy to clipboard operation
radash copied to clipboard

Treat Number Keys as Valid Keys in the set Function

Open nightohl opened this issue 1 year ago • 5 comments

Issue Description:

Currently, the set function does not correctly handle number keys, treating them incorrectly as array indices rather than valid object keys. This causes issues when a path with a number index is provided. The logic needs to be updated to treat number keys as valid keys for objects.

AS-IS

// execute
_.set({}, 'user.1083.name', 'Alice')

// result
{ user: [, , , , , , , , , , , , , , , , , , , , , ,, , , , , , skip , , , , , , , , , , , , , { name: 'Alice'} ] }

TO-BE

// execute
_.set({}, 'user.1083.name', 'Alice')

// result
{ user: { "1083": { name: 'Alice' } } }

Tasks:

  • [x] Modify the logic to handle number keys as valid object keys.
  • [x] Add tests to ensure that number keys are correctly treated as object properties.

nightohl avatar Feb 08 '25 00:02 nightohl

I think you just have to define user: {} before calling set and it will work.

aleclarson avatar Feb 08 '25 00:02 aleclarson

I think you just have to define user: {} before calling set and it will work.

Thank you for your reply! I really appreciate your time in looking into this.

The core issue actually comes from how the path is being parsed. Previously, numeric keys like "1083" were treated as array indices rather than object keys due to how the string was split using /[\.\[\]]/g.

For example:

set({}, 'user.1083.name', 'Alice')

Before (unexpected behavior):

{ user: [, , , , , ..., { name: 'Alice' }] } // user becomes an array

After the fix (expected behavior):

{ user: { "1083": { name: 'Alice' } } } // handled number-key as a key

This update ensures that numeric keys are always treated as object properties, so the function now works correctly even if user isn’t pre-defined. Let me know if you have any other thoughts! 😊

nightohl avatar Feb 08 '25 00:02 nightohl

Your PR changes the path grammar.

If we're gonna make a breaking change, the “path” argument should really be an array. Then we could differentiate "1083" from 1083. It's not like string parsing is more efficient anyway.

There could be a split function for when a path is received over-the-wire and needs to be converted to an array of keys, but I assume most people are using the set utility for local data manipulation (e.g. within a Zustand store).

aleclarson avatar Feb 08 '25 01:02 aleclarson

The docs for set are pretty sparse, but if you look at get, you'll see this example:

import { get } from 'radash'

const fish = {
  name: 'Bass',
  weight: 8,
  sizes: [
    {
      maturity: 'adult',
      range: [7, 18],
      unit: 'inches'
    }
  ]
}

get( fish, 'sizes[0].range[1]' ) // 18
get( fish, 'sizes.0.range.1' ) // 18

So I would consider your PR a breaking change.

aleclarson avatar Feb 08 '25 01:02 aleclarson

@aleclarson Oh, I see what you mean! There's no example in the set documentation for cases like set({}, 'card.0.value', 2). And thinking back to how we used lodash, we had to explicitly use [] brackets in the string approach to reference array values.

So I thought it would be reasonable to treat number-like strings as object keys when they are used without brackets, since there are cases where numbers are used as keys.

But I understand your concern. The get function already has examples of accessing array indices without brackets, so changing this behavior would be a breaking change. I totally agree.

I also really like your idea of using an array as a path. It would allow us to explicitly distinguish "1234" (string) from 1234 (number), making the behavior clearer. Since array-based paths haven’t been supported before, this could be a great starting point to handle them properly with documenting how number keys are handled.

So how about adding support for array paths first? Also, I'd love to hear what’s your opinion on the string-based approach. Should we keep it as-is or gradually transition to the new behavior?

nightohl avatar Feb 08 '25 12:02 nightohl