search

huntabyte / cmdk-sv

cmdk, but for Svelte ✨
https://cmdk-sv.com
MIT License
410 stars 17 forks source link

fix: prevent crashing for non-string value #49

Closed MentalGear closed 5 months ago

changeset-bot[bot] commented 5 months ago

⚠️ No Changeset found

Latest commit: 9168cc66edacd14d5b83725eb973b020e30ba923

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

vercel[bot] commented 5 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
cmdk-sv ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 22, 2024 11:45am
huntabyte commented 5 months ago

The value prop accepts a string type.

If you're experiencing this issue then it should be done outside of the library and in your implementation <Command.Item value={String(whatevermyvalueis)}>.

Adding those chains reduces the perf of the scoring for everyone to allow a few people to pass it unsanitized/checked values. Not worth IMO.

MentalGear commented 5 months ago

It's of course up to you, personally I prefer a robust solution vs negligible performance differences. I'm saying negligible since IMO large lists would be fetched pre-selected from an api and for smaller ones it wouldn't be noticeable by the user.

MentalGear commented 5 months ago

But seems like your approach/use case for this library is different.

huntabyte commented 5 months ago

Right, I'm not saying this shouldn't be possible, it's that the API for value is typed as a string | undefined, which tells the user of the project "do not pass anything that is not a string or undefined to this or it will not work as you expect". By converting whatever value is already supposed to be a string into a string, the API is now lying to the user.

This is problematic because now instead of a runtime error when the user passes in an object as the value, they now have multiple values of [object] [Object], but it appears like everything is working fine until they try to search and it doesn't work how they expect.

Instead, if you did in fact want to pass any arbitrary values to the component, you need to handle the transformations on your side, so that what you are passing to the component aligns with the API.

personally I prefer a robust solution vs negligible performance differences

Regardless of how negligible they are, they are entirely unnecessary at the library level. In no world would you ever need to call String() on a string, which if we look at the types, that's what you're doing in the PR. Even worse, if you passed in undefined, your value is now "undefined" and a bunch of other logic falls apart.