facebook / prop-types

Runtime type checking for React props and similar objects
MIT License
4.48k stars 356 forks source link

Suggestion : PropTypes.forwarded #305

Closed hugomallet closed 4 years ago

hugomallet commented 4 years ago

Hi,

At work we were thinking about defining proptypes of a component that only passes some props to one child. Example :

// Parent.js
const Parent = ({ p1, p2 }) => (
    <div><Child p2={p2} /> {p1}</div>
);

If the proptype of p2 is complex i would prefer to avoid repeating its definition in the proptypes of Parent. So basically i would do this :

// Parent.js
Parent.propTypes = {
    p1: PropTypes.string,
    p2: Child.propTypes.p2,
};

But this can causes problems. For example if child is exported from a HOC such as React.memo :

// Child.js
// ...
export default React.memo(Child); // Child.propTypes are not hoisted

Furthermore i dont want to validate p2 in Parent, it is not necessary because it does not use it. Parent does not care of the type of p2, but it cares that this prop exists and could be required (with isRequired).

PropTypes.any could do the trick but it is perceived (at least by some eslint rules) to be an incomplete proptype information.

This is why i had the idea of a new proptype "PropTypes.forwarded" that would not trigger an eslint error until Parent does not use the corresponding prop directly (ie only passes the prop to another component).

Any thoughts ?

ljharb commented 4 years ago

In Child.js, export default Object.assign(React.memo(Child), Child) or use the hoist-non-react-statics npm package.

How would PropTypes.forwarded get access to Child's propTypes in this case if they weren't hoisted? You'd have to be able to write it yourself for it to be possible for the prop-types library to do it.

hugomallet commented 4 years ago

In Child.js, export default Object.assign(React.memo(Child), Child) or use the hoist-non-react-statics npm package.

I though about it already but i don't like it because it is not universal. You must have control of the Child to correctly export proptypes.

How would PropTypes.forwarded get access to Child's propTypes in this case if they weren't hoisted? You'd have to be able to write it yourself for it to be possible for the prop-types library to do it.

My suggestion is that PropTypes.forwarded is not aware of Child's propTypes. It's like PropTypes.any but with the "additional meaning" that the prop is not used by the component itself.

ljharb commented 4 years ago

If you know a component can be memoized/wrapped in an HOC, then you already should have control of the component.

The point of PropTypes is runtime validation; conveying additional meaning in code with no observable difference seems like territory for code comments?

hugomallet commented 4 years ago

I'm just telling that if the Child component does not exposes correctly its proptypes, you can not reuse them.

About runtime validation, you are right i don't know actually how we could know if the prop is actually used by the component or just passed through :/

ljharb commented 4 years ago

I agree you can't trivially reuse them without altering the Child component; there's not really any way around that, I'm afraid. react-router's Link lacks properly exposed propTypes, and it causes similar issues in codebases that HOC-wrap them, in my experience.

hugomallet commented 4 years ago

And this is an issue that might be avoided because knowing the type of the prop is not necessary.

So PropType.any could do the trick. But the problem that could occur is : if the prop is used in an update later, then the type becomes relevant and the developer wouldn't be notified because the proptype is already defined as any.

This is why i though about adding another validator. But the trick is to check if the prop is actually used or not at runtime ^^

hugomallet commented 4 years ago

Or maybe just an alias that would be used by linters

ljharb commented 4 years ago

If you wanted a linter rule, it could apply to a comment just as easily as an aliased name :-)

hugomallet commented 4 years ago

I am realizing that this is more a linter issue than proptypes one

ljharb commented 4 years ago

I agree; closing for now.