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
Closed
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -135,6 +135,42 @@ describe('PropTypesDevelopmentStandalone', () => {
);
});

it('should warn for missing validator', () => {
spyOn(console, 'error')
const propTypes = { foo: PropTypes.string };
const props = { foo: 'foo', bar: 'bar' };
PropTypes.checkPropTypes(
propTypes,
props,
'prop',
'testComponent',
null,
);
expect(console.error.calls.argsFor(0)[0]).toEqual(
'Warning: Missing `prop-types`: testComponent: prop type `bar` is missing validate in `prop-types`.' +
' Please add type in `prop-types` or remove prop' + ' `bar`' +
' from outer props if not used for performance reason.'
);
});

it('should warn for missing validators', () => {
spyOn(console, 'error')
const propTypes = { foo: PropTypes.string };
const props = { foo: 'foo', bar: 'bar', zoo: 'zoo' };
PropTypes.checkPropTypes(
propTypes,
props,
'prop',
'testComponent',
null,
);
expect(console.error.calls.argsFor(0)[0]).toEqual(
'Warning: Missing `prop-types`: testComponent: prop type `bar`, `zoo` is missing validate in `prop-types`.' +
' Please add type in `prop-types` or remove prop' + ' `bar`, `zoo`' +
' from outer props if not used for performance reason.'
);
})

it('should only warn once for identical validator failures', () => {
spyOn(console, 'error');
const propTypes = { foo: undefined };
@@ -13,7 +13,7 @@ if (process.env.NODE_ENV !== 'production') {
var ReactPropTypesSecret = require('./lib/ReactPropTypesSecret');
var loggedTypeFailures = {};
var has = require('./lib/has');

var diffArr = require('./lib/diffArr');
printWarning = function(text) {
var message = 'Warning: ' + text;
if (typeof console !== 'undefined') {
@@ -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 '
+ 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?

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 diffKeys.join('`, `');
}
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"?

}

/**
@@ -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; });
}

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.

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.