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

Deep validation omits warnings #293

Open
Zyclotrop-j opened this issue Sep 22, 2019 · 2 comments
Open

Deep validation omits warnings #293

Zyclotrop-j opened this issue Sep 22, 2019 · 2 comments

Comments

@Zyclotrop-j
Copy link

Zyclotrop-j commented Sep 22, 2019

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
Copy link
Collaborator

ljharb commented Oct 11, 2019

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

@Tundeh
Copy link

Tundeh commented Jul 30, 2020

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

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

No branches or pull requests

3 participants