Dropdown: Allow null / "" / [] / {} again as option values

VentuzTammoHinrichs commented 3 hours ago

Describe the bug

Before PrimeReact 10, the Dropdown component was able to handle option values like null or empty strings or array or objects, and they worked like any other value.

Since updating to 10, this isn't the case anymore:

The documentation states "If optionValue is omitted and the object has no value property, the object itself becomes the value of an option" but the ObjectUtils.isNotEmpty call at https://github.com/primefaces/primereact/blob/99bc7769f4b7906424fa570c2c60f8a7ce8e02cf/components/lib/dropdown/Dropdown.js#L840 makes sure that this falsely also applies to null and otherwise empty but defined values.
The code at https://github.com/primefaces/primereact/blob/99bc7769f4b7906424fa570c2c60f8a7ce8e02cf/components/lib/dropdown/Dropdown.js#L719 now clears the option selection whenever props.value is null and so prohibits the user from having an option with a null value. I get the reasoning behind this but it would be wiser to do the check only if the search for the option fails, so a null option value would work again.

Reproducer

No response

System Information

System:
    OS: Windows 11 10.0.22631
    CPU: (32) x64 AMD Ryzen 9 7950X 16-Core Processor
    Memory: 12.28 GB / 31.14 GB
  Binaries:
    Node: 20.15.0 - C:\Program Files\nodejs\node.EXE
    npm: 10.7.0 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Edge: Chromium (130.0.2849.52)
    Internet Explorer: 11.0.22621.3527
  npmPackages:
    primereact: ^10.8.2 => 10.8.2
    react: ^18.3.1 => 18.3.1

Steps to reproduce the behavior

Try a Dropdown with the following options:

const options = {
  { label: "auto", value: null },
  { label: "none", value: 0 },
  { label: "one", value: 1 },
  { label: "many", value: 9001 },
}

Expected behavior

IMO null, "", [] and {} should be accepted as option values again - these are perfectly fine, distinct values, and needed in our use case. (as a workaround, I had add two functions that replace those values with dummy strings and then back, and you might agree that's not really elegant)

melloware commented 2 hours ago

Please try 10.8.4 you are using 10.8.2 and i think this was fixed?

github-actions[bot] commented 2 hours ago

Please fork the Stackblitz project and create a case demonstrating your bug report. This issue will be closed if no activities in 20 days.

melloware commented 2 hours ago

I believe was fixed with this: https://github.com/primefaces/primereact/issues/7081

VentuzTammoHinrichs commented 2 hours ago

Please try 10.8.4 you are using 10.8.2 and i think this was fixed?

I'm sorry but the proposed fix for #7081 did sadly not fix our case (note that my source links are form 10.8.4):

You added a check for the optionValue property there. If that property isn't set, and the component defaults back to "value", that isNotEmpty() path still gets taken. Yes, we could work around that by specifying optionValue="value" in all of our numerous DropDown incarnations, but I'd suggest changing this to something like typeof optionValue !== 'undefined' to make the code behave like documented.
The null check in getSelectedOptionIndex still prohibits us from having a null value with a label.

I'll try to create a repro case as suggested.

melloware commented 2 hours ago

Oh in your printout above it says " primereact: ^10.8.2 => 10.8.2"

melloware commented 2 hours ago

PR is welcome

melloware commented 2 hours ago

I assigned to you if you want to submit a PR please

VentuzTammoHinrichs commented 2 hours ago

https://stackblitz.com/edit/vitejs-vite-da87c5?file=src%2FApp.tsx

Ok seems that the first part is indeed fixed in 10.8.4 (seems the optionValue prop is getting filled in with a default so it's always there. I didn't see that, sorry), but the second still remains - if you click on "Auto" in the Dropdown the selection is cleared instead.

melloware commented 2 hours ago

OK cool fix is welcome!

primefaces / primereact