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

PropTypes.array and PropTypes.arrayOf handling with null #19647

Closed
2 tasks done
alexandesigner opened this issue Feb 10, 2020 · 12 comments · Fixed by #19648
Closed
2 tasks done

PropTypes.array and PropTypes.arrayOf handling with null #19647

alexandesigner opened this issue Feb 10, 2020 · 12 comments · Fixed by #19648
Labels
component: autocomplete This is the name of the generic UI component, not the React module! core Infrastructure work going on behind the scenes new feature New feature or request

Comments

@alexandesigner
Copy link
Contributor

alexandesigner commented Feb 10, 2020

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

When filtering an item that does not exist in the list (options), it is returning null and the .filter method is broken

Screen Shot 2020-02-10 at 10 58 24

Expected Behavior 🤔

The ideal would be to check if there are options and if it is an array if it is not going to return an empty array.

Steps to Reproduce 🕹

if you want to reproduce it directly, just go to https://carhoo.com.br/comprar and look for an item when filling out automatic or simple reproduction ... See https://codesandbox.io/s/tender-breeze-gj3zr

Your Environment 🌎

Tech Version
Material-UI v4.9.2
React v16.10.2
Browser Chrome
@oliviertassinari oliviertassinari added status: waiting for author Issue with insufficient information component: autocomplete This is the name of the generic UI component, not the React module! labels Feb 10, 2020
@oliviertassinari
Copy link
Member

Please provide a full reproduction test case. This would help a lot 👷 .
A live example would be perfect. This codesandbox.io template may be a good starting point. Thank you!

@alexandesigner
Copy link
Contributor Author

alexandesigner commented Feb 10, 2020

@oliviertassinari Thanks for the feedback, I'll try to create this reproduction in the codesandbox, however, I provide the link to my production application where the error is occurring.

@alexandesigner
Copy link
Contributor Author

The problem is when the list return is null in my suggestion and the agent just does this little check, maybe I can handle the side of my application however, it would be nice if we could avoid this behavior.

See https://codesandbox.io/s/tender-breeze-gj3zr

@oliviertassinari oliviertassinari added core Infrastructure work going on behind the scenes and removed status: waiting for author Issue with insufficient information component: autocomplete This is the name of the generic UI component, not the React module! labels Feb 10, 2020
@oliviertassinari oliviertassinari changed the title [Autocomplete]: Problem filtering items in the list (options) PropTypes.array and PropTypes.arrayOf handling with null Feb 10, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 10, 2020

@alexandesigner Thanks. I could reproduce the same issue with react-select: https://codesandbox.io/s/react-codesandboxer-example-y2rwr

However, I'm surprised they are no prop-types warnings. null is compatible with PropTypes.array and PropTypes.arrayOf. I see two good options:

  1. We patch prop-types to report a warning.
  2. We accept null. I have looked at the codebase, we have very few cases like this one (3 exactly).
    Autocomplete.options, TreeView.expanded, TablePagination.rowsPerPageOptions,

In any case, the tradeoff is close to #17480. I think that I would be in favor of not crashing here (2.) nor in #17480. A crash forces the developer to solve the issue immediately, while he might prefer to work solve another issue first, and later come back to the warning.

cc @mui-org/core

@eps1lon
Copy link
Member

eps1lon commented Feb 10, 2020

prop-types handling of is required is not ideal but the way the eco-system is set up. With TypeScript you'd already wouldn't pass the type checker. I'd stick with the current behavior to not cause too much confusion. Leveraging default assignments with undefined is a lot easier than having custom default handlers for nullish values.

@oliviertassinari
Copy link
Member

@eps1lon So we give the same answer than #17480 (we close + waiting for upvotes)?

@eps1lon
Copy link
Member

eps1lon commented Feb 10, 2020

Yeah. Since this is the lab I would honestly reconsider making it optional. It doesn't make sense to not set it anyway. If the list is empty (because you're loading) your can default to an empty array.

Makes the type-checker, prop-types and the implementation happy.

Unless there is an actual use case for <Autocomplete /> that never receives options.

@oliviertassinari
Copy link
Member

@alexandesigner What about we make the options required?

@oliviertassinari oliviertassinari added component: autocomplete This is the name of the generic UI component, not the React module! new feature New feature or request labels Feb 10, 2020
@oliviertassinari
Copy link
Member

I have updated the pull request to make the options required, I think that @eps1lon had an excellent point, people won't go far without the options.

@alexandesigner
Copy link
Contributor Author

@oliviertassinari I believe sounds good. I fully agree with the @eps1lon point.

Thanks! Not only for your attention but for the care with this fantastic stack.

@DimaSamusenko
Copy link

Breaking changes as minor version? This was unexpected for me :)

@oliviertassinari
Copy link
Member

The Autocomplete is in an alpha stage, see https://material-ui.com/components/about-the-lab/.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: autocomplete This is the name of the generic UI component, not the React module! core Infrastructure work going on behind the scenes new feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants