Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Treat Number Keys as Valid Keys in the set Function #446

Open
2 tasks done
nightohl opened this issue Feb 8, 2025 · 5 comments
Open
2 tasks done

Treat Number Keys as Valid Keys in the set Function #446

nightohl opened this issue Feb 8, 2025 · 5 comments

Comments

@nightohl
Copy link

nightohl commented Feb 8, 2025

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:

  • Modify the logic to handle number keys as valid object keys.
  • Add tests to ensure that number keys are correctly treated as object properties.
@aleclarson
Copy link

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

@nightohl
Copy link
Author

nightohl commented Feb 8, 2025

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! 😊

@aleclarson
Copy link

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
Copy link

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.

@nightohl
Copy link
Author

nightohl commented Feb 8, 2025

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants