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

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@congwenma
Copy link

congwenma commented Dec 6, 2017

Adds ComponentOfType propType checker.

This helps check Component's type on the receiving end.

//  1. single expected
  propType = {
    label: elementOfType('label')
  }

// 2. multiple expected, multiple given
  propType = {
    children: elementOfType([ MyListItem, 'label' ])
  }

  propType = {
    children: elementOfType([ ...SVG_TAG_GROUP, MyCustomTag ])
  }

@congwenma congwenma force-pushed the congwenma:add-componentOfType branch from b755126 to f2d5902 Dec 6, 2017

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Dec 6, 2017

Technically this is an element, not a component - it's similar to elementType in https://www.npmjs.com/package/airbnb-prop-types.

README.md Outdated
@@ -89,11 +91,14 @@ MyComponent.propTypes = {
// it as an enum.
optionalEnum: PropTypes.oneOf(['News', 'Photos']),
optionalElementOfType: PropTypes.elementOfType(['div', OtherComponent]),

This comment has been minimized.

@ljharb

ljharb Dec 6, 2017

Collaborator

why allow it to take an array of types instead of a single one? PropTypes.oneOfType is the established way to compose multiple validators.

Separately, I think "oneOf" is always in the name when it's taking an array of multiples; thus, I think elementOfType taking one, or oneOfElementType taking an array, is what would make sense - and I think the latter sounds silly.

This comment has been minimized.

@congwenma

congwenma Dec 6, 2017

Author

The specific problem I'm trying to solve is the oneOfType scenario, but considering the need to whitelist more than one type of components:

<CustomTable>
  <CustomHeader/>
  <CustomTableRow/>
  <CustomTableRow/>
  <CustomTableRow/>
  <CustomFooter/>
</CustomTable>

// this is what I'm looking for
CustomTable.propTypes = {
  children: elementsOfType([CustomHeader, CustomTableRow, CustomFooter])
}

or

<SVGSection>
  <use/>
  <g></g>
  <rectangle/>
  <CustomSvgLine />
</SVGSection>

SVGSection.propTypes = {
  children: elementsOfType([ ...SVG_CHILDREN, ...MY_CUSTOM_SVG_CHILDREN ])
}

I'm hoping to capture these use cases so that when allow reusability of component, I'm always covered by this type checker.

This comment has been minimized.

@ljharb

ljharb Dec 6, 2017

Collaborator

PropTypes.oneOfType([ ...SVG_CHILDREN, ...MY_CUSTOM_SVG_CHILDREN ].map(x => PropType.elementOfType(x))) works just fine, without overcomplicating the propType validator.

This comment has been minimized.

@congwenma

congwenma Dec 7, 2017

Author

@ljharb that sounds reasonable, I will make the change.

This comment has been minimized.

@congwenma

congwenma Dec 7, 2017

Author

@ljharb fyi, one feature we will lose from this change is a error message that says expected one of type [div, span, CustomComponent], if we use oneOfType, we will only be seeing the generic message Invalid prop supplied ...

This comment has been minimized.

@ljharb

ljharb Dec 7, 2017

Collaborator

I believe there’s a way oneOfType can read info off of the validators it receives to improve the error message, but I’m not sure what it is.

This comment has been minimized.

@congwenma

congwenma Dec 7, 2017

Author

@ljharb i wonder how that would work if each proptype returns a different error message.

}

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

This comment has been minimized.

@ljharb

ljharb Dec 6, 2017

Collaborator

This should throw if expectedTypes isn't an array (if it takes an array), like oneOf/oneOfType do.

This comment has been minimized.

@congwenma

congwenma Dec 6, 2017

Author

i'm hoping to give user some flexibility in case they only want to pass a single expected element type.

This comment has been minimized.

@ljharb

ljharb Dec 6, 2017

Collaborator

Flexibility in APIs leads to bugs; if they want to pass a single type, they can add [ ] without difficulty.

@@ -871,6 +871,82 @@ describe('PropTypesDevelopmentStandalone', () => {
});
});

describe('ElementOfType Types', () => {

This comment has been minimized.

@ljharb

ljharb Dec 7, 2017

Collaborator

Also, ‘of type” implies it receives propType validators; i think this should be called “elementOf”.

This comment has been minimized.

@congwenma

congwenma Dec 7, 2017

Author

we are really checking if the given object has a .type attribute that matches the given accepted, hows ElementWithType

This comment has been minimized.

@ljharb

ljharb Dec 8, 2017

Collaborator

or maybe just elementType?

This comment has been minimized.

@congwenma

congwenma Dec 8, 2017

Author

actually, there's already a createElementTypeChecker which is a little bit confusing.

This comment has been minimized.

@ljharb

ljharb Dec 8, 2017

Collaborator

createElementTypeTypeChecker :-D

@congwenma congwenma force-pushed the congwenma:add-componentOfType branch from 4a88bbb to 5475382 Dec 7, 2017

@ljharb
Copy link
Collaborator

ljharb left a comment

Can you add a test case that fails with { type: 'div' }? In other words, React.isValidElement should always be true in order for this validator to pass.

@congwenma

This comment has been minimized.

Copy link
Author

congwenma commented Dec 8, 2017

  • Squash
  • Update other specs to match what PropTypesDevelopmentReact15.js covers.
var ACCEPTABLE_TYPES_OF_EXPECTED_TYPES = ['string', 'function'];

if (ACCEPTABLE_TYPES_OF_EXPECTED_TYPES.indexOf(typeof expectedType) === -1) {
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.

@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.

@congwenma

This comment has been minimized.

@ljharb

ljharb Dec 8, 2017

Collaborator

hm, that seems like a missed opportunity.

This comment has been minimized.

@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.

@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.

@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.

@congwenma congwenma force-pushed the congwenma:add-componentOfType branch from 734c4e9 to d0e47ce Dec 21, 2017

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Feb 28, 2019

@congwenma would you mind rebasing this, and addressing outstanding comments?

@congwenma

This comment has been minimized.

Copy link
Author

congwenma commented Mar 4, 2019

@congwenma would you mind rebasing this, and addressing outstanding comments?

@ljharb Rebased! What still needs to be addressed?

@congwenma congwenma force-pushed the congwenma:add-componentOfType branch from d0e47ce to f7f5fc1 Mar 4, 2019

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Mar 7, 2019

@congwenma #146 (comment) in particular

Show resolved Hide resolved factoryWithTypeCheckers.js Outdated
Show resolved Hide resolved factoryWithTypeCheckers.js Outdated
Show resolved Hide resolved factoryWithTypeCheckers.js Outdated
}

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

This comment has been minimized.

@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.

@congwenma

congwenma Mar 7, 2019

Author

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

This comment has been minimized.

@ljharb

ljharb Mar 8, 2019

Collaborator

oneOfType, yes.

Show resolved Hide resolved factoryWithTypeCheckers.js Outdated

ljharb and others added some commits Mar 7, 2019

Update factoryWithTypeCheckers.js
Co-Authored-By: congwenma <congwen.ma@gmail.com>
Update factoryWithTypeCheckers.js
Co-Authored-By: congwenma <congwen.ma@gmail.com>
Update factoryWithTypeCheckers.js
Co-Authored-By: congwenma <congwen.ma@gmail.com>
Update factoryWithTypeCheckers.js
Co-Authored-By: congwenma <congwen.ma@gmail.com>
@@ -51,6 +51,7 @@ module.exports = function() {
objectOf: getShim,
oneOf: getShim,
oneOfType: getShim,
elementWithType: getShim,

This comment has been minimized.

@ljharb

ljharb Mar 8, 2019

Collaborator

also let’s name this elementType

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.