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.

Sign up for GitHub

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

Jump to bottom

[Add ComponentOfType] #146

Closed
wants to merge 6 commits into from
Closed

[Add ComponentOfType] #146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f7f5fc1
[Add ComponentOfType]
congwenma Dec 6, 2017
0135063
Update factoryWithTypeCheckers.js
ljharb Mar 7, 2019
25f5467
Update factoryWithTypeCheckers.js
ljharb Mar 7, 2019
d8d77b3
Update factoryWithTypeCheckers.js
ljharb Mar 7, 2019
4a43d01
Update factoryWithTypeCheckers.js
ljharb Mar 7, 2019
9823dfa
updated some arrow functions to function declarations, added ReactIs
congwenma Mar 31, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 6 additions & 1 deletion README.md
View file
Open in desktop
Expand Up @@ -63,6 +63,8 @@ class MyComponent extends React.Component {
}
}

const OtherComponent = () => <div/>

MyComponent.propTypes = {
// You can declare that a prop is a specific JS primitive. By default, these
// are all optional.
Expand Down Expand Up @@ -93,11 +95,14 @@ MyComponent.propTypes = {
// it as an enum.
optionalEnum: PropTypes.oneOf(['News', 'Photos']),

optionalElementWithType: PropTypes.elementWithType(['div', OtherComponent]),

// An object that could be one of many types
optionalUnion: PropTypes.oneOfType([
PropTypes.string,
PropTypes.number,
PropTypes.instanceOf(Message)
PropTypes.instanceOf(Message),
PropTypes.elementWithType(['label'])
]),

// An array of a certain type
Expand Down
94 changes: 94 additions & 0 deletions __tests__/PropTypesDevelopmentReact15.js
View file
Open in desktop
Expand Up @@ -1044,6 +1044,100 @@ describe('PropTypesDevelopmentReact15', () => {
});
});

describe('ElementWithType Types', () => {
it('should fail for mismatched component.type', () => {
typeCheckFail(
PropTypes.elementWithType('div'),
<span/>,
"Warning: Failed prop type: Invalid prop `testProp` of type [span] supplied to `testComponent`, expected one of type `div`."
);
});

it('should pass for matched component.type', () => {
typeCheckPass(PropTypes.elementWithType('div'), <div />);
});

it('should pass if no component given', () => {
ljharb marked this conversation as resolved.
Show resolved Hide resolved
typeCheckPass(PropTypes.elementWithType('div'));
});

describe('factory error', () => {
it('should fail if using any value thats not a function nor a string for expectedTypes', () => {
ljharb marked this conversation as resolved.
Show resolved Hide resolved
spyOn(console, 'error');

PropTypes.elementWithType(1);

expect(console.error).toHaveBeenCalled();
expect(console.error.calls.argsFor(0)[0]).toContain(
"Invalid argument supplied to ElementWithType, expected an Html tag name or a Component."
);

typeCheckPass(PropTypes.elementWithType(1), <span />);
});
});

describe('with .isRequired', () => {
it('should fail if not passing in anything', () => {
typeCheckFail(
PropTypes.elementWithType('div').isRequired,
null,
"Warning: Failed prop type: The prop `testProp` is marked as required in `testComponent`, but its value is `null`."
);
});
});

describe('custom component', () => {
const MyComponent = () => <div></div>;
var myComponentPropType;
beforeEach(() => {
myComponentPropType = PropTypes.elementWithType(MyComponent)
});

it('should fail for mismatched component.type', () => {
typeCheckFail(
myComponentPropType,
<span />,
"Warning: Failed prop type: Invalid prop `testProp` of type [span] supplied to `testComponent`, expected one of type `MyComponent`."
);
});

it('should pass for matched component.type', () => {
typeCheckPass(myComponentPropType, <MyComponent />);
});
});

describe('given component mismatches with expected', () => {
it('should warn if given invalid element', () => {
[
'<div></div>',
{ type: 'div' },
1,
['div'],
'div'
].forEach(invalidElement =>
typeCheckFail(
PropTypes.elementWithType('div'),
invalidElement,
"Warning: Failed prop type: Invalid prop `testProp` of component `testComponent` has been given invalid component."
)
);
});
});

describe('list of expected types', () => {
it('should warn if expected type does not include given component.type', () => {
typeCheckFail(
PropTypes.elementWithType('div'),
<span />,
"Warning: Failed prop type: Invalid prop `testProp` of type [span] supplied to `testComponent`, expected one of type `div`."
);
});
it('should pass if expected type include given component.type', () => {
typeCheckPass(PropTypes.elementWithType('div'), <div />)
});
});
});

describe('Union Types', () => {
it('should warn but not error for invalid argument', () => {
spyOn(console, 'error');
Expand Down
94 changes: 94 additions & 0 deletions __tests__/PropTypesDevelopmentStandalone-test.js
View file
Open in desktop
Expand Up @@ -1118,6 +1118,100 @@ describe('PropTypesDevelopmentStandalone', () => {
});
});

describe('ElementWithType Types', () => {
it('should fail for mismatched component.type', () => {
typeCheckFail(
PropTypes.elementWithType('div'),
<span />,
"Warning: Failed prop type: Invalid prop `testProp` of type [span] supplied to `testComponent`, expected one of type `div`."
);
});

it('should pass for matched component.type', () => {
typeCheckPass(PropTypes.elementWithType('div'), <div />);
});

it('should pass if no component given', () => {
typeCheckPass(PropTypes.elementWithType('div'));
});

describe('factory error', () => {
it('should fail if using any value thats not a function nor a string for expectedTypes', () => {
spyOn(console, 'error');

PropTypes.elementWithType(1);

expect(console.error).toHaveBeenCalled();
expect(console.error.calls.argsFor(0)[0]).toContain(
"Invalid argument supplied to ElementWithType, expected an Html tag name or a Component."
);

typeCheckPass(PropTypes.elementWithType(1), <span />);
});
});

describe('with .isRequired', () => {
it('should fail if not passing in anything', () => {
typeCheckFail(
PropTypes.elementWithType('div').isRequired,
null,
"Warning: Failed prop type: The prop `testProp` is marked as required in `testComponent`, but its value is `null`."
);
});
});

describe('custom component', () => {
const MyComponent = () => <div></div>;
var myComponentPropType;
beforeEach(() => {
myComponentPropType = PropTypes.elementWithType(MyComponent)
});

it('should fail for mismatched component.type', () => {
typeCheckFail(
myComponentPropType,
<span />,
"Warning: Failed prop type: Invalid prop `testProp` of type [span] supplied to `testComponent`, expected one of type `MyComponent`."
);
});

it('should pass for matched component.type', () => {
typeCheckPass(myComponentPropType, <MyComponent />);
});
});

describe('given component mismatches with expected', () => {
it('should warn if given invalid element', () => {
[
'<div></div>',
{ type: 'div' },
1,
['div'],
'div'
].forEach(invalidElement =>
typeCheckFail(
PropTypes.elementWithType('div'),
invalidElement,
"Warning: Failed prop type: Invalid prop `testProp` of component `testComponent` has been given invalid component."
)
);
});
});

describe('list of expected types', () => {
it('should warn if expected type does not include given component.type', () => {
typeCheckFail(
PropTypes.elementWithType('div'),
<span />,
"Warning: Failed prop type: Invalid prop `testProp` of type [span] supplied to `testComponent`, expected one of type `div`."
);
});
it('should pass if expected type include given component.type', () => {
typeCheckPass(PropTypes.elementWithType('div'), <div />)
});
});
});

describe('Union Types', () => {
it('should warn but not error for invalid argument', () => {
spyOn(console, 'error');
Expand Down
90 changes: 90 additions & 0 deletions __tests__/PropTypesProductionReact15-test.js
View file
Open in desktop
Expand Up @@ -800,6 +800,96 @@ describe('PropTypesProductionReact15', () => {
});
});

describe('ElementWithType Types', () => {
it('should fail for mismatched component.type', () => {
expectNoop(
PropTypes.elementWithType('div'),
<span />,
"Warning: Failed prop type: Invalid prop `testProp` of type [span] supplied to `testComponent`, expected one of type `div`."
);
});

it('should pass for matched component.type', () => {
expectNoop(PropTypes.elementWithType('div'), <div />);
});

it('should pass if no component given', () => {
expectNoop(PropTypes.elementWithType('div'));
});

describe('factory error', () => {
it('should fail if using any value thats not a function nor a string for expectedTypes', () => {
spyOn(console, 'error');

PropTypes.elementWithType(1);

expect(console.error).not.toHaveBeenCalled();
expectNoop(PropTypes.elementWithType(1), <span />);
});
});

describe('with .isRequired', () => {
it('should fail if not passing in anything', () => {
expectNoop(
PropTypes.elementWithType('div').isRequired,
null,
"Warning: Failed prop type: The prop `testProp` is marked as required in `testComponent`, but its value is `null`."
);
});
});

describe('custom component', () => {
const MyComponent = () => <div></div>;
var myComponentPropType;
beforeEach(() => {
myComponentPropType = PropTypes.elementWithType(MyComponent)
});

it('should fail for mismatched component.type', () => {
expectNoop(
myComponentPropType,
<span />,
"Warning: Failed prop type: Invalid prop `testProp` of type [span] supplied to `testComponent`, expected one of type `MyComponent`."
);
});

it('should pass for matched component.type', () => {
expectNoop(myComponentPropType, <MyComponent />);
});
});

describe('given component mismatches with expected', () => {
it('should warn if given invalid element', () => {
[
'<div></div>',
{ type: 'div' },
1,
['div'],
'div'
].forEach(invalidElement =>
expectNoop(
PropTypes.elementWithType('div'),
invalidElement,
"Warning: Failed prop type: Invalid prop `testProp` of component `testComponent` has been given invalid component."
)
);
});
});

describe('list of expected types', () => {
it('should warn if expected type does not include given component.type', () => {
expectNoop(
PropTypes.elementWithType('div'),
<span />,
"Warning: Failed prop type: Invalid prop `testProp` of type [span] supplied to `testComponent`, expected one of type `div`."
);
});
it('should pass if expected type include given component.type', () => {
expectNoop(PropTypes.elementWithType('div'), <div />)
});
});
});

describe('Union Types', () => {
it('should ignore invalid argument', () => {
spyOn(console, 'error');
Expand Down
10 changes: 10 additions & 0 deletions __tests__/PropTypesProductionStandalone-test.js
View file
Open in desktop
Expand Up @@ -194,6 +194,16 @@ describe('PropTypesProductionStandalone', function() {
});
});

describe('elementWithType Types', () => {
it('should be a no-op', () => {
expectThrowsInProduction(PropTypes.elementWithType(1));
expectThrowsInProduction(PropTypes.elementWithType('div'), 'div');
expectThrowsInProduction(PropTypes.elementWithType('div'), { type: 'div' });
expectThrowsInProduction(PropTypes.elementWithType(['div', 'span']), <div/>);
expectThrowsInProduction(PropTypes.elementWithType('div'), [<div/>, <span/>]);
});
});

describe('Union Types', function() {
it('should be a no-op', function() {
expectThrowsInProduction(
Expand Down
1 change: 1 addition & 0 deletions factoryWithThrowingShims.js
View file
Open in desktop
Expand Up @@ -51,6 +51,7 @@ module.exports = function() {
objectOf: getShim,
oneOf: getShim,
oneOfType: getShim,
elementWithType: getShim,
Copy link
Collaborator

Choose a reason for hiding this comment

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

also let’s name this elementType

Copy link
Author

Choose a reason for hiding this comment

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

elementType already exist, looks like this PR beated us to it. b67bbd4.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Whoops, indeed - looks like #211 may have replaced this.

shape: getShim,
exact: getShim,

Expand Down
33 changes: 33 additions & 0 deletions factoryWithTypeCheckers.js
View file
Open in desktop
Expand Up @@ -129,6 +129,7 @@ module.exports = function(isValidElement, throwOnDirectAccess) {
node: createNodeChecker(),
objectOf: createObjectOfTypeChecker,
oneOf: createEnumTypeChecker,
elementWithType: createElementWithTypeChecker,
oneOfType: createUnionTypeChecker,
shape: createShapeTypeChecker,
exact: createStrictShapeTypeChecker,
Expand Down Expand Up @@ -364,6 +365,38 @@ module.exports = function(isValidElement, throwOnDirectAccess) {
return createChainableTypeChecker(validate);
}


function createElementWithTypeChecker(expectedType) {
var ACCEPTABLE_TYPES_OF_EXPECTED_TYPES = ['string', 'function'];

if (!ReactIs.isValidElementType(expectedType)) {
process.env.NODE_ENV !== 'production' ? warning(false, 'Invalid argument supplied to ElementWithType, expected an Html tag name or a Component.') : void 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should not use warning here; I'm suggesting this have a literal throw statement.

Copy link
Author

Choose a reason for hiding this comment

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

this would be different from the behaviors of other type checkers.

https://github.com/congwenma/prop-types/blob/add-componentOfType/factoryWithTypeCheckers.js#L277

Copy link
Collaborator

Choose a reason for hiding this comment

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

hm, that seems like a missed opportunity.

Copy link
Author

Choose a reason for hiding this comment

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

wouldn't this be a useful pattern by not making a big deal about prop types in production?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure what you mean; invalid props are bugs. The point of not warning about them in production is performance; the exception I'm hoping for here would only apply to dev anyways, since the entire propType validator is compiled out anyways in production.

Copy link
Author

@congwenma congwenma Dec 10, 2017 •
edited

Choose a reason for hiding this comment

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

sorry I thought you mean replacing the entire line with a throw.
I think what you are suggesting can make things cleaner and easier, plus we'd be removing dependency on fbjs/warning, however I think theres some logic in fbjs/warning that helps debugging prop-types and handle some special cases, and this probably warrants a separate PR.

return emptyFunction.thatReturnsNull;
}

function getDisplayName(comp) {
return typeof comp === 'function' ? comp.displayName || comp.name : comp;
}

function validate(props, propName, componentName, location, propFullName) {
var propValues = [].concat(props[propName]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should just be props[propName]; there's no reason to support multiple values here.

Copy link
Author

Choose a reason for hiding this comment

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

Are you saying the support of multiple element types should be using oneOf?

Copy link
Collaborator

@ljharb ljharb Mar 8, 2019 •
edited

Choose a reason for hiding this comment

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

oneOfType, yes.


if (propValues.some(function (pv) { return !isValidElement(pv); })) {
return new PropTypeError('Invalid ' + location + ' `' + propFullName + '` of component `' + componentName + '` has been given invalid component.');
}

var hasInvalid = propValues.some(function(propValue) {
return expectedType !== propValue.type
});
if (hasInvalid) {
return new PropTypeError('Invalid ' + location + ' `' + propFullName + '` of type [' + propValues.map(function (pv) { return getDisplayName(pv.type) }).join(', ') + '] ' + ('supplied to `' + componentName + '`, expected one of type `' + getDisplayName(expectedType) + '`.'));
}

return null;
}
return createChainableTypeChecker(validate)
}

function createUnionTypeChecker(arrayOfTypeCheckers) {
if (!Array.isArray(arrayOfTypeCheckers)) {
process.env.NODE_ENV !== 'production' ? printWarning('Invalid argument supplied to oneOfType, expected an instance of array.') : void 0;
Expand Down