facebook / prop-types

Runtime type checking for React props and similar objects
MIT License
4.48k stars 356 forks source link

Deep validation omits warnings #293

Open Zyclotrop-j opened 5 years ago

Zyclotrop-j commented 5 years ago

Setup / Replicate

Have a component with deep propTypes:

static propTypes = {
str1: PropTypes.string.isRequired, // Shallow
str2: PropTypes.string.isRequired, // Shallow
test: PropTypes.shape({
    str3: PropTypes.string.isRequired, // Deep
    str4: PropTypes.string.isRequired // Deep
})
}

Render that component so that the deep propTypes are propType-invalid (eg <Example test={{}} /> for this example) [tr1, str2, str3 and str4 are all required but not present, so all four are invalid.]

Expected

PropTypes warnings for all failed propTypes For the example above this would be:

Warning: Failed prop type: The prop `str1` is marked as required in `Validator`, but its value is `undefined`.
Warning: Failed prop type: The prop `str2` is marked as required in `Validator`, but its value is `undefined`.
Warning: Failed prop type: The prop `str3` is marked as required in `Validator`, but its value is `undefined`.
Warning: Failed prop type: The prop `str4` is marked as required in `Validator`, but its value is `undefined`.

Actual

For deep propTypes only the first error is logged out, validation stops after that. On the other hand, this is not true for shallow properties (like str1 and str2).

For the example above this is:

Warning: Failed prop type: The prop `str1` is marked as required in `Validator`, but its value is `undefined`.
Warning: Failed prop type: The prop `str2` is marked as required in `Validator`, but its value is `undefined`.
Warning: Failed prop type: The prop `str3` is marked as required in `Validator`, but its value is `undefined`.
/* Missing warning for str4! */

Note

This doesn't only trigger on required, but also on everything else, like .number, .string, ...

Second example

static propTypes = {
str1: PropTypes.number, // Shallow
str2: PropTypes.string, // Shallow
test: PropTypes.shape({
    str3: PropTypes.number, // Deep
    str4: PropTypes.string // Deep
})
}

+

<Example str1={""} str2={1} test={{ str3: "", str4: 1 }} />

Full code (for copy-and-paste)

import React, { PureComponent } from "react";
import PropTypes from 'prop-types';

class Example extends PureComponent {
  render() {
    return (<h1>Example</h1>);
  }

  static propTypes = {
    str: PropTypes.string.isRequired,
    str2: PropTypes.string.isRequired,
    test: PropTypes.shape({
      str3: PropTypes.string.isRequired,
      str4: PropTypes.string.isRequired
    })
  }
ReactDOM.render(<Example test={{}} />, document.getElementById('root'));

Cause of the bug

https://github.com/facebook/prop-types/blob/master/factoryWithTypeCheckers.js#L433 -> Exists on the first find opposed to https://github.com/facebook/prop-types/blob/master/checkPropTypes.js#L44 -> loops through all, no return to be seen [React calls this internally on validation, see https://github.com/facebook/react/blob/master/packages/react/src/ReactElementValidator.js#L218]

ljharb commented 5 years ago

This seems like a good enhancement (not a bugfix or a breaking change) - to print out more errors and be more helpful here.

Tundeh commented 4 years ago

can i take on this issue? also, i'm new here,. if you have a specific implementation in mind.