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

setKeys() and setItems() do not support ArrayLike Iterables because they use [].concat() for a shallow copy #20

Closed
SamuelTrew opened this issue Apr 6, 2022 · 3 comments
Assignees
Labels
bug Something isn't working

Comments

@SamuelTrew
Copy link

If I were to call setItems with something like a mobx ObservableArray, to JS it is an iterable object.
So when concat is called at the beginning of the setItems function, it doesn't properly concat, due to not being an actual array. (turns into an array that contains a mobx array that contains an object).

I know this is not exactly the libraries fault, but it would be nice to support ArrayLike Iterables.

I suggest that the spread operator is used instead to copy:

[...items] as opposed to [].concat(items)

Thanks!

@fwextensions
Copy link
Owner

The idea with this parameter handling was that you could just pass a bare string as the key to score, if you only needed one, rather than an array with one item. The problem with using the spread operator on a string is that [..."foo"] gives you ["f", "o", "o"], which is obviously not what you want.

The constructor requires a single key string be in an array, and it's always been doc'd as an array in setKeys(), so I should just remove that special handling.

@fwextensions fwextensions self-assigned this Apr 6, 2022
@fwextensions fwextensions added the bug Something isn't working label Apr 6, 2022
@fwextensions fwextensions changed the title setItems does not support ArrayLike Iterables setKeys() does not support ArrayLike Iterables Apr 6, 2022
fwextensions added a commit that referenced this issue Apr 7, 2022
@SamuelTrew
Copy link
Author

Ahhh yeah I forgot about the string case.
I also thought that the point might have been to use a copy of the keys and not the reference? But I don't think it matters in this case right?

Also, I realised I muddled the title and the text, I was referring to setItems when also setKeys also does it 😅 .
Would setItems be able to be updated as well?
Thanks!

@fwextensions
Copy link
Owner

You're right, setItems creates a copy of the array as well. I think I was initially avoiding the spread operator/Array.from() for wider compatibility, and avoiding .slice(0) to handle a single bare string. Seems like using slice is the simplest and most compatible solution for making a shallow copy, so I'll make that change.

@fwextensions fwextensions changed the title setKeys() does not support ArrayLike Iterables setKeys() and setItems() do not support ArrayLike Iterables because they use [].concat() for a shallow copy Apr 7, 2022
fwextensions added a commit that referenced this issue Apr 7, 2022
Use .slice(0) instead, so that it works with more array-like things.
Resolve #20.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants