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

`checkPropTypes`: add warning for props that not validate in prop-types #272

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@Zaynex
Copy link

commented May 9, 2019

Hi, this pull request can help developers to distinguish the props that not validate.
Here is example:

function App() {
  const [node, setNode ] = useState(1);
  return <div onClick={() => setNode(Math.random())}>Hi<Child child={'child'} node={node} /></div>
}

class Child extends PureComponent {
  render() {
    console.log('render');
    return <div>{this.props.child}</div>
  }
}
Child.propTypes = {
  child: PropTypes.number,
}

Even if Child component is pureComponent, it still cause Child re-render if unused props change(node in example).

@facebook-github-bot

This comment has been minimized.

Copy link

commented May 9, 2019

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot

This comment has been minimized.

Copy link

commented May 9, 2019

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@ljharb
Copy link
Collaborator

left a comment

to be honest, this seems like it's doing what PropTypes.exact / https://npmjs.com/prop-types-exact does - have you looked into those?

@@ -0,0 +1,3 @@
module.exports = function diffArr(base, target) {
return base.filter(function(item){return target.indexOf(item) < 0});
}

This comment has been minimized.

Copy link
@ljharb

ljharb May 10, 2019

Collaborator
Suggested change
}
}

This comment has been minimized.

Copy link
@Zaynex

Zaynex May 11, 2019

Author

Yes. But I haven't heard of it before. my pull request and PropTypes.extra have a common goal to warn those outer types not validate in child propsTypes. So developers can know how many props actually we use, and remove unnecessary props.

This comment has been minimized.

Copy link
@ljharb

ljharb May 11, 2019

Collaborator

right - but i think doing that by default would be quite noisy, and a breaking change, whereas the component author already has the ability via exact to achieve the same goal.

This comment has been minimized.

Copy link
@Zaynex

Zaynex May 11, 2019

Author

well, I have a question:
If you add extra in your project, are you both have extra and no extra components ?
I think those who notice extra, are sure to want to check all props.Even we have React.memo and PureComponent, It will still cause re-render when unsed props change.

And by the way, what is prop-types aim? It's only check the props you use or check the props from outer? the PropTypes.x.isRequired warn developer I must have value from parent. why we shouldn't warn developer that props not validated in child 😬?

This comment has been minimized.

Copy link
@ljharb

ljharb May 11, 2019

Collaborator

(exact, not extra)

Yes, this is true - generally that's why you'd want to use a linter rule to require all your components use exact props. There are many components - like react-router's Link - that would break with this restriction.

This comment has been minimized.

Copy link
@Zaynex

Zaynex May 11, 2019

Author

My consideration is not enough, thank you for giving me a negative example and your patience.
Do you think should we add exact feature in prop-types?so we don't have to introduce two libraries.

import PropTypes, { exact } from 'prop-types';

This comment has been minimized.

Copy link
@ljharb

ljharb May 11, 2019

Collaborator

It’s already there - the separate library just predates PropTypes.exact

This comment has been minimized.

Copy link
@Zaynex

Zaynex May 11, 2019

Author

Now I think I should close this pr. My careless about the documentation.
thx.

@@ -0,0 +1,3 @@
module.exports = function diffArr(base, target) {
return base.filter(function(item){return target.indexOf(item) < 0});

This comment has been minimized.

Copy link
@ljharb

ljharb May 10, 2019

Collaborator
Suggested change
return base.filter(function(item){return target.indexOf(item) < 0});
return base.filter(function (item) { return target.indexOf(item) < 0; });
var componentKeys = Object.keys(componentProps);
var propKeys = Object.keys(propTypes);
var diffKeys = diffArr(componentKeys, propKeys);
if(diffKeys.length) {

This comment has been minimized.

Copy link
@ljharb

ljharb May 10, 2019

Collaborator
Suggested change
if(diffKeys.length) {
if (diffKeys.length > 0) {
}
return false;

This comment has been minimized.

Copy link
@ljharb

ljharb May 10, 2019

Collaborator
Suggested change
return false;
return '';

should this return an empty string, rather than being "string or false"?

@@ -86,7 +86,26 @@ function checkPropTypes(typeSpecs, values, location, componentName, getStack) {
}
}
}

var missKeys = checkIfPropsMissKey(values, typeSpecs);
if(missKeys){

This comment has been minimized.

Copy link
@ljharb

ljharb May 10, 2019

Collaborator
Suggested change
if(missKeys){
if (missKeys) {
printWarning(
'Missing `prop-types`: '+
(componentName || 'React class') + ': ' + location + ' type `'+ missKeys +
'` is missing validate in `prop-types`. Please add type in `prop-types` or remove '

This comment has been minimized.

Copy link
@ljharb

ljharb May 10, 2019

Collaborator
Suggested change
'` is missing validate in `prop-types`. Please add type in `prop-types` or remove '
'` is missing validation in `.propTypes`. Please add a validator in `.propTypes` or remove '
'Missing `prop-types`: '+
(componentName || 'React class') + ': ' + location + ' type `'+ missKeys +
'` is missing validate in `prop-types`. Please add type in `prop-types` or remove '
+ location + ' `' + missKeys + '` from outer props if not used for performance reason.');

This comment has been minimized.

Copy link
@ljharb

ljharb May 10, 2019

Collaborator
Suggested change
+ location + ' `' + missKeys + '` from outer props if not used for performance reason.');
+ location + ' `' + missKeys + '` from outer props if not used.');
function checkIfPropsMissKey(componentProps, propTypes) {
var componentKeys = Object.keys(componentProps);
var propKeys = Object.keys(propTypes);
var diffKeys = diffArr(componentKeys, propKeys);

This comment has been minimized.

Copy link
@ljharb

ljharb May 10, 2019

Collaborator

i think it'd be better to inline this, since it's only used once.

This comment has been minimized.

Copy link
@Zaynex

Zaynex May 11, 2019

Author

I need the function name to comment self .Should I move checkIfPropsMissKey in checkPropTypes?

@Zaynex Zaynex closed this May 11, 2019

@Zaynex

This comment has been minimized.

Copy link
Author

commented May 11, 2019

  1. Already implementation in current version.
  2. break change the default behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.