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

[core] Components crash when required prop is missing #17480

Closed
fredprodibi opened this issue Sep 18, 2019 · 5 comments
Closed

[core] Components crash when required prop is missing #17480

fredprodibi opened this issue Sep 18, 2019 · 5 comments
Labels

Comments

@fredprodibi
Copy link

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

Current Behavior 😯

if ariaLabel is missing the component crashes

Expected Behavior 🤔

Steps to Reproduce 🕹

take the demo and remove ariaLabel

Steps:

Context 🔦

Your Environment 🌎

Tech Version
Material-UI v4.?.?
React
Browser
TypeScript
etc.
@mbrookes mbrookes added bug 🐛 Something doesn't work component: speed dial This is the name of the generic UI component, not the React module! labels Sep 18, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 19, 2019

This prop is required. We have other components that crash when a required prop is missing or wrong, this problem is not specific to the Speed Dial. Could you expand on the concern? Thanks.

The list of components that crash on missing required props (as far as I'm aware of):

  • SpeedDial
  • ExpansionPanel
  • Tooltip
  • FormControlLabel
  • Modal
  • TrapFocus

Note that you won't see the prop-type warning when rendering on the server.

@oliviertassinari oliviertassinari added discussion and removed bug 🐛 Something doesn't work component: speed dial This is the name of the generic UI component, not the React module! labels Sep 19, 2019
@oliviertassinari oliviertassinari changed the title [speed-dial] When ariaLabel is missing the component crashes [core] Components crash when required prop is missing Sep 19, 2019
@fredprodibi
Copy link
Author

fredprodibi commented Sep 19, 2019

Hello, when you know it it's fine.

It's my first project with meterial UI and it wasn't simple to figure out what was wrong as the error wasn't "requiered prop ariaLabel is missing", but something like "replace is not a method of undefined"

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 19, 2019

It's my first project with meterial UI and it wasn't simple to figure out what was wrong as the error wasn't "requiered prop ariaLabel is missing", but something like "replace is not a method of undefined"

@fredprodibi Thanks! It seems that the warning message gets buried down by the exception and its stack trace.

I'm in favor of adding defensive checks in the codebase to avoid these crashes and have the prop types warning messages easier to follow. What do you think @eps1lon?

@eps1lon
Copy link
Member

eps1lon commented Sep 19, 2019

It's my first project with meterial UI and it wasn't simple to figure out what was wrong as the error wasn't "requiered prop ariaLabel is missing", but something like "replace is not a method of undefined"

I understand your confusion. This caused by the error screen of create react app only having a tiny hint to check your console. This also might happen when you stop reading the log when the last error occured. I would recommend to either start at the top of your error log or don't stop reading when you see the first error log.

In this specific example above the "replace is not a method of undefined" an additional error should be logged telling you that Warning: Failed prop type: The prop ariaLabel is marked as required in ForwardRef(SpeedDial), but its value is undefined.

@oliviertassinari
Copy link
Member

I think that we can keep the components crash when they miss important props.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants