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

[Add ComponentOfType] #146

Closed
wants to merge 6 commits into from
Closed
Diff settings

Always

Just for now

@@ -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.
@@ -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
@@ -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', () => {
This conversation was marked as resolved by ljharb

This comment has been minimized.

Copy link
@ljharb

ljharb Dec 8, 2017

Collaborator

This needs a test case for the required version that fails

This comment has been minimized.

Copy link
@congwenma

congwenma Dec 8, 2017

Author

updated. I'm adding a task to cover the "syching specs" part.

typeCheckPass(PropTypes.elementWithType('div'));
});

describe('factory error', () => {
it('should fail if using any value thats not a function nor a string for expectedTypes', () => {
This conversation was marked as resolved by ljharb

This comment has been minimized.

Copy link
@ljharb

ljharb Dec 8, 2017

Collaborator

Wouldn’t this throw, synchronously?

This comment has been minimized.

Copy link
@congwenma

congwenma Dec 8, 2017

Author

what do you mean by that?

This comment has been minimized.

Copy link
@ljharb

ljharb Dec 8, 2017

Collaborator

I mean that PropTypes.elementWithType(x) should throw, immediately, if x is not a string or a function - it shouldn’t have to wait until propType validations to do so.

This comment has been minimized.

Copy link
@congwenma

congwenma Dec 8, 2017

Author

yes, updated factory spec.

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');
@@ -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');
@@ -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');
@@ -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(
@@ -51,6 +51,7 @@ module.exports = function() {
objectOf: getShim,
oneOf: getShim,
oneOfType: getShim,
elementWithType: getShim,

This comment has been minimized.

Copy link
@ljharb

ljharb Mar 8, 2019

Collaborator

also let’s name this elementType

This comment has been minimized.

Copy link
@congwenma

congwenma Mar 31, 2019

Author

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

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 1, 2019

Collaborator

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

shape: getShim,
exact: getShim,

@@ -129,6 +129,7 @@ module.exports = function(isValidElement, throwOnDirectAccess) {
node: createNodeChecker(),
objectOf: createObjectOfTypeChecker,
oneOf: createEnumTypeChecker,
elementWithType: createElementWithTypeChecker,
oneOfType: createUnionTypeChecker,
shape: createShapeTypeChecker,
exact: createStrictShapeTypeChecker,
@@ -364,6 +365,36 @@ module.exports = function(isValidElement, throwOnDirectAccess) {
return createChainableTypeChecker(validate);
}


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

if (ACCEPTABLE_TYPES_OF_EXPECTED_TYPES.indexOf(typeof expectedType) === -1) {
This conversation was marked as resolved by congwenma

This comment has been minimized.

Copy link
@ljharb

ljharb Mar 7, 2019

Collaborator

instead of making an array and using indexOf, this should repeat typeof here - it's actually optimized to be faster anyways. However, instead, why not:

Suggested change
if (ACCEPTABLE_TYPES_OF_EXPECTED_TYPES.indexOf(typeof expectedType) === -1) {
if (!isValidElementType(expectedType)) {

? (using isValidElement from react-is)?

process.env.NODE_ENV !== 'production' ? warning(false, 'Invalid argument supplied to ElementWithType, expected an Html tag name or a Component.') : void 0;

This comment has been minimized.

Copy link
@ljharb

ljharb Dec 8, 2017

Collaborator

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

This comment has been minimized.

Copy link
@congwenma

This comment has been minimized.

Copy link
@ljharb

ljharb Dec 8, 2017

Collaborator

hm, that seems like a missed opportunity.

This comment has been minimized.

Copy link
@congwenma

congwenma Dec 9, 2017

Author

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

This comment has been minimized.

Copy link
@ljharb

ljharb Dec 9, 2017

Collaborator

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.

This comment has been minimized.

Copy link
@congwenma

congwenma Dec 10, 2017

Author

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) {
This conversation was marked as resolved by congwenma

This comment has been minimized.

Copy link
@ljharb

ljharb Mar 7, 2019

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

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

This comment has been minimized.

Copy link
@ljharb

ljharb Mar 7, 2019

Collaborator

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

This comment has been minimized.

Copy link
@congwenma

congwenma Mar 7, 2019

Author

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

This comment has been minimized.

Copy link
@ljharb

ljharb Mar 8, 2019

Collaborator

oneOfType, yes.


if (propValues.some(pv => !isValidElement(pv))) {
This conversation was marked as resolved by congwenma

This comment has been minimized.

Copy link
@ljharb

ljharb Mar 7, 2019

Collaborator
Suggested change
if (propValues.some(pv => !isValidElement(pv))) {
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(propValue => expectedType !== propValue.type);
if (hasInvalid) {
return new PropTypeError('Invalid ' + location + ' `' + propFullName + '` of type [' + propValues.map(pv => pv.type).map(getDisplayName).join(', ') + '] ' + ('supplied to `' + componentName + '`, expected one of type `' + getDisplayName(expectedType) + '`.'));
This conversation was marked as resolved by congwenma

This comment has been minimized.

Copy link
@ljharb

ljharb Mar 7, 2019

Collaborator

we can't use arrow functions in this package, since it doesn't use babel.

Suggested change
return new PropTypeError('Invalid ' + location + ' `' + propFullName + '` of type [' + propValues.map(pv => pv.type).map(getDisplayName).join(', ') + '] ' + ('supplied to `' + componentName + '`, expected one of type `' + getDisplayName(expectedType) + '`.'));
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;
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.