Fix #7187: Provide readable input value based on selected option labels

[iamkyrylo]

Fix #7187

[vercel]

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

2 Skipped Deployments

Name	Status	Preview	Comments	Updated (UTC)
primereact	⬜️ Ignored (Inspect)	Visit Preview		Sep 15, 2024 11:03am
primereact-v9	⬜️ Ignored (Inspect)	Visit Preview		Sep 15, 2024 11:03am

[github-actions]

Thanks a lot for your contribution! But, PR does not seem to be linked to any issues. Please manually link to an issue or mention it in the description using #<issue_id>.

[github-actions]

Thanks a lot for your contribution! But, PR does not seem to be linked to any issues. Please manually link to an issue or mention it in the description using #<issue_id>.

[github-actions]

Thanks a lot for your contribution! But, PR does not seem to be linked to any issues. Please manually link to an issue or mention it in the description using #<issue_id>.

[melloware]

@iamkyrylo thank you I assigned to PrimeTek for review.

[nitrogenous]

lgtm

[ivanpajon]

Removing line with JSON.stringify breaks this fix #6897
@melloware @iamkyrylo

[iamkyrylo]

Removing line with JSON.stringify breaks this fix #6897

I'm not sure I understand the issue. This doesn’t break anything. The input value shouldn’t consist of serialized objects with all their attributes. Instead, the input value should be a simple string made up of the option labels, as this is what users see on the screen and select from the dropdown.

[ivanpajon]

The issue is that not serializing the value to the input causes this:

And if you are using the component inside a form tag for example you will submit [object Object], [object Object] instead of the correct value of the input:

I don't know if there is another solution to address this but now (without serialize) doesn't work with forms.

[iamkyrylo]

@ivanpajon Please take a closer look at the changes in this PR, specifically this line. With these changes, based on your example, the input value will be a simple string like value="Module 1, Module 2" instead of value="[object Object], [object Object]".

[ivanpajon]

@iamkyrylo yes, you are right, I have don't check it, sorry.

In my case I need to use a custom object as the value so I think now I should use useOptionAsValue prop. I have check it with useOptionAsValue prop and in this case input value is [object Object], [object Object]. You can see it in this reproducer I made.

[iamkyrylo]

Oh, I see what you mean and understand the problem. It’s not related to the useOptionAsValue prop. It comes from checking props.optionLabel in the condition. Try to add optionLabel and you will see that everything works as expected, with or without useOptionAsValue.

I was misled by the component types, which indicated that optionLabel has a default value of label. But, since the component is implemented in JS, it would need to have defaultProps defined to align with the type declaration file, but it doesn’t - there are no propTypes or defaultProps🤷‍♂️

Anyway, I can make a fix and remove this check from the condition.

[ivanpajon]

With optionLabel it works but not as intended.
I mean, if I set useOptionAsValue I want the whole object to be the value, which in the reproducer is doing it well but without serialize it. That is why input has [object Object], [object Object] as value instead of [{"id":"fd9jlbv3s","name":"city_one","label":"City One"},{"id":"sdvb56n5f","name":"city_two","label":"City Two"}].

Expected behavior would be to get [{"id":"fd9jlbv3s","name":"city_one","label":"City One"},{"id":"sdvb56n5f","name":"city_two","label":"City Two"}] as input value.

[iamkyrylo]

This approach is technically incorrect. Exposing the entire value in the dom is not good for accessibility and security. You should keep in mind that the input value in the dom is for users, not developers. If you need to handle the option object, store it in the component's state rather than serializing/deserializing a JSON string from the dom.

I’m not sure why the useOptionAsValue prop was added, but if it’s for the reason you mentioned, I'd suggest that the core team remove it to avoid confusion and maintain better accessibility practices. The MultiSelect component already allows you to manage the selected option directly using the option object when optionValue is not provided.

I’d like to get the core team's thoughts on this. @melloware @nitrogenous

[melloware]

totally agree with @iamkyrylo JSON in a readable element is wrong and a code smell.

[iamkyrylo]

I've submitted a follow-up fix - #7216

[ivanpajon]

I also don't like the approach with serialization.
I only want to mention that without that this component would never work with NextJS form action submissions. At least without extra boilerplate.

[iamkyrylo]

@ivanpajon I'm going to point this out again so you don't confuse or mislead yourself or others — it's not recommended to serialize objects for the input value, regardless of which form implementation you're using (NextJS, Remix, native, etc.). Since MultiSelect is a controlled component, you need to manage its state anyway. Also, MultiSelect allows you to handle objects directly to define the selected options. So, there's no problem using this state for the request instead of relying solely on the formData object.

Check NextJS documentation again, and you’ll find a convenient solution that suits your case.