tbleckert / react-select-search

⚡️ Lightweight select component for React
https://react-select-search.com
MIT License
676 stars 149 forks source link

fix issue #140 #141

Closed chuksjoe closed 3 years ago

chuksjoe commented 3 years ago

When setting the initial value of a multiple select input to an array of values, it usually an error. This PR fixes that error.

vercel[bot] commented 3 years ago

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/tbleckert/react-select-search/n9gu7eods
✅ Preview: https://react-select-search-git-fix-initial-array-value-bug.tbleckert.now.sh

tbleckert commented 3 years ago

Thanks for the fix @chuksjoe ! I think it might be worth exploring why undefined values reaches getDisplayValue in the first place. I think the real error comes earlier in the chain. Haven't looked into it yet but I think getNewValue might be a good start.

tbleckert commented 3 years ago

@chuksjoe Looks like we exit early if no old value, but doesn't handle arrays here https://github.com/tbleckert/react-select-search/blob/master/src/lib/getNewValue.js#L7 . Should be something like:

if (!oldValue) {
    return !Array.isArray(value) ? [value] : value;
}
chuksjoe commented 3 years ago

At the point where it checks the initial values against the options if the options is not gotten immediately (that is if the options list is dependent on an API call), the values in the list will be undefined and trying to get the name in getDisplayValue() will fail. So checking if the value exists (singleOption && singleOption.name) helps prevent the whole code from breaking.

tbleckert commented 3 years ago

@chuksjoe Yes, I just meant that falsy values shouldn't even be in the array and reach getDisplayValue. So the fix is more a patch. For example, the getOption produces falsy values and sends those to getDisplayValue (https://github.com/tbleckert/react-select-search/blob/master/src/useSelect.js#L51-L61).

This has been refactored quite a bit in the next branch. So the fix is only needed in getNewValue, sorry for the confusion, got a bit ahead of myself.

But let's merge this in and publish as this fixes the issue for now.

chuksjoe commented 3 years ago

Yea. that's great. Thanks