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

fix: allow object options to share the same label #217

Merged
merged 3 commits into from
Mar 25, 2023
Merged

fix: allow object options to share the same label #217

merged 3 commits into from
Mar 25, 2023

Conversation

GauBen
Copy link
Contributor

@GauBen GauBen commented Mar 24, 2023

Closes #216

Keys can be objects, though somewhat discouraged.

The key can be any object, but strings and numbers are recommended since they allow identity to persist when the objects themselves change.

@janosh
Copy link
Owner

janosh commented Mar 24, 2023

I thought keys have to be primitive types?

@janosh
Copy link
Owner

janosh commented Mar 24, 2023

Ah, I was wrong. From the Svelte docs:

You can use any object as the key, as Svelte uses a Map internally — in other words you could do (thing) instead of (thing.id). Using a string or number is generally safer, however, since it means identity persists without referential equality, for example when updating with fresh data from an API server.

@GauBen
Copy link
Contributor Author

GauBen commented Mar 24, 2023

Svelte uses a Map under the hood, objects are legal keys but can have unexpected side effect, that's why the doc discourage non primitive keys

Edit: ahahah you were faster

@janosh
Copy link
Owner

janosh commented Mar 24, 2023

Maybe we should still default to the option label as key but allow overriding this behavior by exposing a new prop get_key: (option: Option) => unknown = get_label?

@GauBen
Copy link
Contributor Author

GauBen commented Mar 25, 2023

That's a an option too, but having two different values with the same name would still break svelte-multiselect

@janosh
Copy link
Owner

janosh commented Mar 25, 2023

That's true but that was by design (until now anyway). Maybe it's not a good default. I think I made that decision at the time because I couldn't think of a strong use case for duplicate options. That was before allowing user-created options. But even now, maybe it's better to show a warning to users rather than allowing them to make duplicate options?

@GauBen
Copy link
Contributor Author

GauBen commented Mar 25, 2023

That's right, shall I update this PR with a new get_key option? Or are you ok with option being used as an index?

The only thing that might break with the current PR is animate:flip when selected is updated by copy and not reference

https://svelte.dev/repl/60e7b5893b304b8f9e0ea094fe6bb293?version=3.57.0

(just updated the repl with both kind of copies)

@janosh
Copy link
Owner

janosh commented Mar 25, 2023

The only thing that might break with the current PR is animate:flip when selected is updated by copy and not reference

I didn't realize the downside was this small. Nice REPL btw. Very educational.

That's right, shall I update this PR with a new get_key option?

No, I'm sold. I think we should just add a test that covers selecting two identical object options.

@janosh janosh added testing Test all the things discussion Gathering feedback on open questions labels Mar 25, 2023
@janosh janosh enabled auto-merge (squash) March 25, 2023 03:57
@janosh janosh merged commit 8340b7f into janosh:main Mar 25, 2023
@GauBen
Copy link
Contributor Author

GauBen commented Mar 26, 2023

Thanks for the quick merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Gathering feedback on open questions testing Test all the things
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error: Cannot have duplicate keys in a keyed each when two labels are the same
2 participants