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

PropTypes: distinguish nullable from optional object field #7291

Merged
merged 3 commits into from
Jul 26, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 14 additions & 8 deletions src/addons/link/__tests__/ReactLinkPropTypes-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ var ReactPropTypeLocations = require('ReactPropTypeLocations');
var ReactPropTypesSecret = require('ReactPropTypesSecret');

var invalidMessage = 'Invalid prop `testProp` supplied to `testComponent`.';
var requiredMessage =
'Required prop `testProp` was not specified in `testComponent`.';
var requiredMessage = 'The prop `testProp` is marked as required in ' +
'`testComponent`, but its value is `undefined`.';

function typeCheckFail(declaration, value, message) {
var props = {testProp: value};
Expand Down Expand Up @@ -53,22 +53,26 @@ describe('ReactLink', function() {
typeCheckFail(
LinkPropTypes.link(React.PropTypes.any),
{},
'Required prop `testProp.value` was not specified in `testComponent`.'
'The prop `testProp.value` is marked as required in `testComponent`, ' +
'but its value is `undefined`.'
);
typeCheckFail(
LinkPropTypes.link(React.PropTypes.any),
{value: 123},
'Required prop `testProp.requestChange` was not specified in `testComponent`.'
'The prop `testProp.requestChange` is marked as required in ' +
'`testComponent`, but its value is `undefined`.'
);
typeCheckFail(
LinkPropTypes.link(React.PropTypes.any),
{requestChange: emptyFunction},
'Required prop `testProp.value` was not specified in `testComponent`.'
'The prop `testProp.value` is marked as required in `testComponent`, ' +
'but its value is `undefined`.'
);
typeCheckFail(
LinkPropTypes.link(React.PropTypes.any),
{value: null, requestChange: null},
'Required prop `testProp.value` was not specified in `testComponent`.'
'The prop `testProp.value` is marked as required in `testComponent`, ' +
'but its value is `null`.'
);
});

Expand Down Expand Up @@ -122,12 +126,14 @@ describe('ReactLink', function() {
});

it('should warn for missing required values', function() {
typeCheckFail(LinkPropTypes.link().isRequired, null, requiredMessage);
var specifiedButIsNullMsg = 'The prop `testProp` is marked as required ' +
'in `testComponent`, but its value is `null`.';
typeCheckFail(LinkPropTypes.link().isRequired, null, specifiedButIsNullMsg);
typeCheckFail(LinkPropTypes.link().isRequired, undefined, requiredMessage);
typeCheckFail(
LinkPropTypes.link(React.PropTypes.string).isRequired,
null,
requiredMessage
specifiedButIsNullMsg
);
typeCheckFail(
LinkPropTypes.link(React.PropTypes.string).isRequired,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,8 @@ describe('ReactContextValidator', function() {
expect(console.error.calls.count()).toBe(1);
expect(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe(
'Warning: Failed context type: ' +
'Required context `foo` was not specified in `Component`.\n' +
'The context `foo` is marked as required in `Component`, but its value ' +
'is `undefined`.\n' +
' in Component (at **)'
);

Expand Down Expand Up @@ -229,7 +230,8 @@ describe('ReactContextValidator', function() {
expect(console.error.calls.count()).toBe(1);
expect(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe(
'Warning: Failed childContext type: ' +
'Required child context `foo` was not specified in `Component`.\n' +
'The child context `foo` is marked as required in `Component`, but its ' +
'value is `undefined`.\n' +
' in Component (at **)'
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -360,8 +360,8 @@ describe('ReactElementValidator', function() {

expect(console.error.calls.count()).toBe(1);
expect(console.error.calls.argsFor(0)[0]).toBe(
'Warning: Failed prop type: ' +
'Required prop `prop` was not specified in `Component`.\n' +
'Warning: Failed prop type: The prop `prop` is marked as required in ' +
'`Component`, but its value is `null`.\n' +
' in Component'
);
});
Expand All @@ -385,8 +385,8 @@ describe('ReactElementValidator', function() {

expect(console.error.calls.count()).toBe(1);
expect(console.error.calls.argsFor(0)[0]).toBe(
'Warning: Failed prop type: ' +
'Required prop `prop` was not specified in `Component`.\n' +
'Warning: Failed prop type: The prop `prop` is marked as required in ' +
'`Component`, but its value is `null`.\n' +
' in Component'
);
});
Expand All @@ -413,7 +413,8 @@ describe('ReactElementValidator', function() {
expect(console.error.calls.count()).toBe(2);
expect(console.error.calls.argsFor(0)[0]).toBe(
'Warning: Failed prop type: ' +
'Required prop `prop` was not specified in `Component`.\n' +
'The prop `prop` is marked as required in `Component`, but its value ' +
'is `undefined`.\n' +
' in Component'
);

Expand Down
10 changes: 8 additions & 2 deletions src/isomorphic/classic/types/ReactPropTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -144,9 +144,15 @@ function createChainableTypeChecker(validate) {
if (props[propName] == null) {
var locationName = ReactPropTypeLocationNames[location];
if (isRequired) {
if (props[propName] === null) {
return new Error(
`The ${locationName} \`${propFullName}\` is marked as required ` +
`in \`${componentName}\`, but its value is \`null\`.`
);
}
return new Error(
`Required ${locationName} \`${propFullName}\` was not specified in ` +
`\`${componentName}\`.`
`The ${locationName} \`${propFullName}\` is marked as required in ` +
`\`${componentName}\`, but its value is \`undefined\`.`
);
}
return null;
Expand Down
130 changes: 58 additions & 72 deletions src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ var ReactPropTypesSecret;

var Component;
var MyComponent;
var requiredMessage =
'Required prop `testProp` was not specified in `testComponent`.';

function typeCheckFail(declaration, value, message) {
var props = {testProp: value};
Expand All @@ -37,6 +35,46 @@ function typeCheckFail(declaration, value, message) {
expect(error.message).toBe(message);
}

function typeCheckFailRequiredValues(declaration) {
var specifiedButIsNullMsg = 'The prop `testProp` is marked as required in ' +
'`testComponent`, but its value is `null`.';
var unspecifiedMsg = 'The prop `testProp` is marked as required in ' +
'`testComponent`, but its value is \`undefined\`.';
var props1 = {testProp: null};
var error1 = declaration(
props1,
'testProp',
'testComponent',
ReactPropTypeLocations.prop,
null,
ReactPropTypesSecret
);
expect(error1 instanceof Error).toBe(true);
expect(error1.message).toBe(specifiedButIsNullMsg);
var props2 = {testProp: undefined};
var error2 = declaration(
props2,
'testProp',
'testComponent',
ReactPropTypeLocations.prop,
null,
ReactPropTypesSecret
);
expect(error2 instanceof Error).toBe(true);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit but I guess you’d want to do the instanceof check for error1 too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, corrected, thanks.

expect(error2.message).toBe(unspecifiedMsg);
var props3 = {};
var error3 = declaration(
props3,
'testProp',
'testComponent',
ReactPropTypeLocations.prop,
null,
ReactPropTypesSecret
);
expect(error3 instanceof Error).toBe(true);
expect(error3.message).toBe(unspecifiedMsg);
}

function typeCheckPass(declaration, value) {
var props = {testProp: value};
var error = declaration(
Expand Down Expand Up @@ -146,8 +184,7 @@ describe('ReactPropTypes', function() {
});

it('should warn for missing required values', function() {
typeCheckFail(PropTypes.string.isRequired, null, requiredMessage);
typeCheckFail(PropTypes.string.isRequired, undefined, requiredMessage);
typeCheckFailRequiredValues(PropTypes.string.isRequired);
});

it('should warn if called manually in development', function() {
Expand Down Expand Up @@ -213,8 +250,7 @@ describe('ReactPropTypes', function() {
});

it('should warn for missing required values', function() {
typeCheckFail(PropTypes.any.isRequired, null, requiredMessage);
typeCheckFail(PropTypes.any.isRequired, undefined, requiredMessage);
typeCheckFailRequiredValues(PropTypes.any.isRequired);
});

it('should warn if called manually in development', function() {
Expand Down Expand Up @@ -307,15 +343,8 @@ describe('ReactPropTypes', function() {
});

it('should warn for missing required values', function() {
typeCheckFail(
PropTypes.arrayOf(PropTypes.number).isRequired,
null,
requiredMessage
);
typeCheckFail(
PropTypes.arrayOf(PropTypes.number).isRequired,
undefined,
requiredMessage
typeCheckFailRequiredValues(
PropTypes.arrayOf(PropTypes.number).isRequired
);
});

Expand Down Expand Up @@ -406,8 +435,7 @@ describe('ReactPropTypes', function() {
});

it('should warn for missing required values', function() {
typeCheckFail(PropTypes.element.isRequired, null, requiredMessage);
typeCheckFail(PropTypes.element.isRequired, undefined, requiredMessage);
typeCheckFailRequiredValues(PropTypes.element.isRequired);
});

it('should warn if called manually in development', function() {
Expand Down Expand Up @@ -493,12 +521,7 @@ describe('ReactPropTypes', function() {
});

it('should warn for missing required values', function() {
typeCheckFail(
PropTypes.instanceOf(String).isRequired, null, requiredMessage
);
typeCheckFail(
PropTypes.instanceOf(String).isRequired, undefined, requiredMessage
);
typeCheckFailRequiredValues(PropTypes.instanceOf(String).isRequired);
});

it('should warn if called manually in development', function() {
Expand Down Expand Up @@ -601,16 +624,7 @@ describe('ReactPropTypes', function() {
});

it('should warn for missing required values', function() {
typeCheckFail(
PropTypes.node.isRequired,
null,
'Required prop `testProp` was not specified in `testComponent`.'
);
typeCheckFail(
PropTypes.node.isRequired,
undefined,
'Required prop `testProp` was not specified in `testComponent`.'
);
typeCheckFailRequiredValues(PropTypes.node.isRequired);
});

it('should accept empty array for required props', function() {
Expand Down Expand Up @@ -724,15 +738,8 @@ describe('ReactPropTypes', function() {
});

it('should warn for missing required values', function() {
typeCheckFail(
PropTypes.objectOf(PropTypes.number).isRequired,
null,
requiredMessage
);
typeCheckFail(
PropTypes.objectOf(PropTypes.number).isRequired,
undefined,
requiredMessage
typeCheckFailRequiredValues(
PropTypes.objectOf(PropTypes.number).isRequired
);
});

Expand Down Expand Up @@ -804,16 +811,7 @@ describe('ReactPropTypes', function() {
});

it('should warn for missing required values', function() {
typeCheckFail(
PropTypes.oneOf(['red', 'blue']).isRequired,
null,
requiredMessage
);
typeCheckFail(
PropTypes.oneOf(['red', 'blue']).isRequired,
undefined,
requiredMessage
);
typeCheckFailRequiredValues(PropTypes.oneOf(['red', 'blue']).isRequired);
});

it('should warn if called manually in development', function() {
Expand Down Expand Up @@ -882,15 +880,8 @@ describe('ReactPropTypes', function() {
});

it('should warn for missing required values', function() {
typeCheckFail(
PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired,
null,
requiredMessage
);
typeCheckFail(
PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired,
undefined,
requiredMessage
typeCheckFailRequiredValues(
PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired
);
});

Expand Down Expand Up @@ -950,7 +941,8 @@ describe('ReactPropTypes', function() {
typeCheckFail(
PropTypes.shape({key: PropTypes.number.isRequired}),
{},
'Required prop `testProp.key` was not specified in `testComponent`.'
'The prop `testProp.key` is marked as required in `testComponent`, ' +
'but its value is `undefined`.'
);
});

Expand All @@ -961,7 +953,8 @@ describe('ReactPropTypes', function() {
secondKey: PropTypes.number.isRequired,
}),
{},
'Required prop `testProp.key` was not specified in `testComponent`.'
'The prop `testProp.key` is marked as required in `testComponent`, ' +
'but its value is `undefined`.'
);
});

Expand All @@ -983,15 +976,8 @@ describe('ReactPropTypes', function() {
});

it('should warn for missing required values', function() {
typeCheckFail(
PropTypes.shape({key: PropTypes.number}).isRequired,
null,
requiredMessage
);
typeCheckFail(
PropTypes.shape({key: PropTypes.number}).isRequired,
undefined,
requiredMessage
typeCheckFailRequiredValues(
PropTypes.shape({key: PropTypes.number}).isRequired
);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -249,8 +249,8 @@ describe('ReactJSXElementValidator', function() {
expect(
console.error.calls.argsFor(0)[0].replace(/\(at .+?:\d+\)/g, '(at **)')
).toBe(
'Warning: Failed prop type: ' +
'Required prop `prop` was not specified in `RequiredPropComponent`.\n' +
'Warning: Failed prop type: The prop `prop` is marked as required in ' +
'`RequiredPropComponent`, but its value is `null`.\n' +
' in RequiredPropComponent (at **)'
);
});
Expand All @@ -264,8 +264,8 @@ describe('ReactJSXElementValidator', function() {
expect(
console.error.calls.argsFor(0)[0].replace(/\(at .+?:\d+\)/g, '(at **)')
).toBe(
'Warning: Failed prop type: ' +
'Required prop `prop` was not specified in `RequiredPropComponent`.\n' +
'Warning: Failed prop type: The prop `prop` is marked as required in ' +
'`RequiredPropComponent`, but its value is `null`.\n' +
' in RequiredPropComponent (at **)'
);
});
Expand All @@ -281,7 +281,8 @@ describe('ReactJSXElementValidator', function() {
console.error.calls.argsFor(0)[0].replace(/\(at .+?:\d+\)/g, '(at **)')
).toBe(
'Warning: Failed prop type: ' +
'Required prop `prop` was not specified in `RequiredPropComponent`.\n' +
'The prop `prop` is marked as required in `RequiredPropComponent`, but ' +
'its value is `undefined`.\n' +
' in RequiredPropComponent (at **)'
);

Expand Down
4 changes: 2 additions & 2 deletions src/test/__tests__/ReactTestUtils-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -296,8 +296,8 @@ describe('ReactTestUtils', function() {
expect(
console.error.calls.argsFor(0)[0].replace(/\(at .+?:\d+\)/g, '(at **)')
).toBe(
'Warning: Failed context type: Required context `name` was not ' +
'specified in `SimpleComponent`.\n' +
'Warning: Failed context type: The context `name` is marked as ' +
'required in `SimpleComponent`, but its value is `undefined`.\n' +
' in SimpleComponent (at **)'
);
});
Expand Down