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 a PropTypes for React elementType (ie. MyComponent) #211

Merged
merged 1 commit into from Feb 10, 2019

Conversation

Projects
None yet
7 participants
@eXon
Copy link
Contributor

eXon commented Sep 9, 2018

There is a missing prop type that is quite common for a lot of people: PropTypes.component. The PropTypes.element already exists to support <MyComponent myProp={<MyOtherComponent />} />. However, there is no way to easily type check: <MyComponent myProp={MyOtherComponent} />. Before React 16.3, we were getting away with PropTypes.func.

ForwardRef in 16.3

However, with new features since React 16.3, there is a new challenge: forward ref! forwardRef is generating a React component, but in a different form: it is an object! This is breaking the old way of using the PropTypes.func and making the type checking unreliable. To fix this, we need a more complex prop type:

MyComponent.propTypes = {
  myProp: PropTypes.oneOf([
    PropTypes.func,
    PropTypes.shape({
      $$typeof: PropTypes.symbol
    })
  ]).isRequired
};

The problem with this:

  1. Now we're leaking an internal implementation of React
  2. If you pass an element (<MyComponent />) instead of a component (MyComponent), it would not fail. An addition check on the $$typeof value has to be made and now the complexity is getting out of hand for a prop type that should be simple.

To fix this, I've added PropTypes.component that would fix this problem by checking the $$typeof and only allow components.

Higher-Order Component (HOC)

The reason why this prop type is so important to add is because of HOC. If you're using a library HOC, as soon as you add the decorator on your component using forwardRef (example react-cookie, see reactivestack/cookies#172), your component is an object instead of a function and most libraries (example react-router) are using PropTypes.func to check if your passing a component. The typecheck wrongfully fails.

What should be a component?

First we need to support regular stateless and stateful component. The best way to do that is by checking if it's a function. I don't think we can do better here unfortunately because of stateless components.

Then, there is special cases created by React like the forward ref. Here are all the $$typeof we can check and which one should be a component or not:

  • REACT_PROVIDER_TYPE: Yes

  • REACT_CONTEXT_TYPE: Yes

  • REACT_FORWARD_REF_TYPE: Yes

  • REACT_ELEMENT_TYPE: No

  • REACT_PORTAL_TYPE: No

  • REACT_FRAGMENT_TYPE: No

  • REACT_STRICT_MODE_TYPE: No

  • REACT_PROFILER_TYPE: No

  • REACT_ASYNC_MODE_TYPE: No

  • REACT_PLACEHOLDER_TYPE`: No

Forward ref is the most obvious use-case, but the context provider/consumer are technically component created by React. I just don't think it would be used much in a prop though.

Remaining Concerns

Here are my main concerns with this PR:

Bumping React version to 16 in devDependencies

The tests were running on the old 15 version of React, I'm guessing to make sure we don't break the old version. However, to test forward ref and the context API, we need at least React 16.3. I've updated the version, but an alternative would be to have two different package with the different version. Is it worth it? Also, should we bump the version of prop-types to 16? There is no breaking change, but the tests would be running on that version unless we run the tests on both by adding a proxy package pointing to React 15.

Should we really call that new prop type component?

I think it's the best name, but in the previous code, the prop type element (and sometimes node) were referred as component. To me, a React component is MyComponent while a React element is <MyComponent />. Is it so for the community? Is the distinction clear enough?

EDIT: Been renamed elementType to follow is-react name.

Where I've added isValidComponent

Looking at the isValidElement code, I tried to stay consistent with where to put the code. However, I'm not sure this is 100% clean. I can refactor this if you think this is a problem, but I've tried to stay consistent with the current state of the code.

Should we have more validation?

We could add more validation. Is the component should be a specific one? Should the component have a specific set of prop type (not sure about the use cases)?

Documentation

If this is merged, we would need to update the React documentation with the new prop type. I can help with that once this is merged.

Fixes #223.

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Sep 9, 2018

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Sep 9, 2018

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

index.js Outdated
var isValidComponent = function(component) {
// Stateless or stateful component
if (typeof component === 'function') {
return true;

This comment has been minimized.

@ljharb

ljharb Sep 9, 2018

Collaborator

this seems like it would return a lot of false positives, but i don't know of a better way to cover SFCs. It seems like an early return true for class-based components, though, would be better. You may want to use the react-is package here, and check the various "is" methods contained within.

This comment has been minimized.

@eXon

eXon Sep 9, 2018

Author Contributor

I agree, it was my first though but went for a first version before digging into this. If it's a function (not a class), it should accept it. If it's a class, we should check if it inherits React.Component or from React.createClass. Not sure if I can do this with enough reliability.

index.js Outdated
@@ -11,16 +11,56 @@ if (process.env.NODE_ENV !== 'production') {
Symbol.for('react.element')) ||
0xeac7;

var REACT_PROVIDER_TYPE =

This comment has been minimized.

@ljharb

ljharb Sep 9, 2018

Collaborator

these should be pulled from react-is and not hardcoded here.

This comment has been minimized.

@eXon

eXon Sep 9, 2018

Author Contributor

I would have to add the react-is as a production dependency, are you open to that? I saw you were already hardcoding the element type. I wouldn't mind switching all of them.

This comment has been minimized.

@ljharb

ljharb Sep 9, 2018

Collaborator

i'm not a collaborator on this repo, but personally yes, i am - it's minified heavily in production, and you could also wrap the require in a NODE_ENV check.

@eXon

This comment has been minimized.

Copy link
Contributor Author

eXon commented Sep 9, 2018

This is related to #200. Would probably fix the issue.

README.md Outdated
@@ -81,6 +81,9 @@ MyComponent.propTypes = {
// A React element.
optionalElement: PropTypes.element,
// A React component.
optionalComponent: PropTypes.component,

This comment has been minimized.

@ljharb

ljharb Sep 9, 2018

Collaborator

this needs to be renamed also?

@eXon

This comment has been minimized.

Copy link
Contributor Author

eXon commented Sep 9, 2018

Turns out is-react have everything elementType needs. Even though we depend on the 16.5.0 version, I don't think it would have any problem to work on older versions including 15.

@eXon eXon changed the title Add a PropTypes for React component MyComponent Add a PropTypes for React elementType (ie. MyComponent) Sep 9, 2018

@eXon eXon referenced this pull request Sep 10, 2018

Closed

Fix <Route component /> type #6327

@taion

This comment has been minimized.

Copy link

taion commented Sep 10, 2018

BTW, we've had this implemented in prop-types-extra as elementType for a while. We've found it helpful to include an additional warning when the user accidentally passes in an element: https://github.com/react-bootstrap/prop-types-extra/blob/v1.1.0/src/elementType.js.

@eedrah

This comment has been minimized.

Copy link

eedrah commented Oct 1, 2018

Closed issue #223 in favor of this pull request.

@aldeed

This comment has been minimized.

Copy link

aldeed commented Dec 5, 2018

@ljharb Anything community can do to help get this merged and released? Code appears similar to working code that I had previously posted and to the prop-types-extra code mentioned, and your earlier comments appear to have been addressed. In general LGTM as far as fixing my #200 issue.

@ljharb

This comment was marked as outdated.

Copy link
Collaborator

ljharb commented Dec 5, 2018

@aldeed i'm not a maintainer of this repo, so I'm not really sure.

@NSLS

This comment has been minimized.

Copy link

NSLS commented Dec 6, 2018

Can we get some attention from the maintainers?

Without this PR we can't get rid of the PropTypes warnings from third-party packages, as they all rely on a component being either string, node or function.

@ljharb

ljharb approved these changes Feb 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment