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

[Select] Support object value #13661

Merged
merged 2 commits into from
Nov 23, 2018

Conversation

yezhi780625
Copy link
Contributor

@yezhi780625 yezhi780625 commented Nov 21, 2018

In Select component, the property value support object type. The value will pass into Input and InputBase components. But it is not supported in Input and InputBase .

What's more, I think that the propertydefaultValue should support the same types as property value.

Closes #10845

@oliviertassinari oliviertassinari self-assigned this Nov 21, 2018
@oliviertassinari oliviertassinari added new feature New feature or request component: select This is the name of the generic UI component, not the React module! labels Nov 21, 2018
@oliviertassinari oliviertassinari changed the title [Input, InputBase] fix propTypes of value and defaultValue [Select] Support object value Nov 21, 2018
@oliviertassinari oliviertassinari force-pushed the fix/input/prop-types branch 2 times, most recently from 1b01bc9 to f6ea91b Compare November 21, 2018 23:01
@oliviertassinari oliviertassinari removed their assignment Nov 21, 2018
@@ -133,7 +133,15 @@ Input.propTypes = {
/**
* The default input value, useful when not controlling the component.
*/
defaultValue: PropTypes.oneOfType([PropTypes.string, PropTypes.number]),
defaultValue: PropTypes.oneOfType([
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will voice my concerns once again because this is increasingly magic since the value will be cast to string. People should do that explicitly so that the component appears to be less magic. Allowing numbers is already bad as it is since numbers will go in but strings will come out.

Especially for something like objects it doesn't make much sense to allow them since most of them will generate the same value ([object Object]).

Copy link
Member

@oliviertassinari oliviertassinari Nov 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan either, but many people ask for it. At this point, we might just use any for the propType, like React is doing for <input>. Now, most people use an XHR to send the form values to the server. They don't care about the [object Object] DOM casting. Also, as we don't cast objet to string in the non native select implementation, this works.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well if you access event.target.value then you should care about casting. I was hoping we could add a more predictable wrapper around input since type coercion is imo one of the bigger issues in JS since it can't be addressed with linters.

But since we already allowed this with numbers we might as well be consistent. PropTypes.any it is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having object select support definitely comes with a cost :/. Does it worth it?

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 23, 2018

I'm going to merge this pull-request. To sum up:

  • The native select implementation doesn't support object value. It's not possible, and that's for the best.
  • The non-native select implementation (default) supports it but comes with a tradeoff. It can't be used for any standard client -> server form posting. The object value will be cast to "[object Object]" then rendered into the DOM as a string. The native input React elements don't raise any warning about it: https://codesandbox.io/s/o4w168k2lz. I have looked at react-select, they don't even have any propTypes. In some way, this change makes us less opinionated. I haven't added any documentation for this behavior, I would rather not encourage it and keep it "under the radar".

@oliviertassinari oliviertassinari merged commit b9d757c into mui:master Nov 23, 2018
@yezhi780625 yezhi780625 deleted the fix/input/prop-types branch November 27, 2018 02:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: select This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants