facebook / prop-types

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

Add prop type for Refs #240

Open mindtraveller opened 5 years ago

mindtraveller commented 5 years ago

How about adding prop types for Refs as a built in feature? Right now every *ref property looks like this:

buttonRef: PropTypes.oneOfType([
    PropTypes.func, // for legacy refs
    PropTypes.shape({ current: PropTypes.instanceOf(Element) })
])

Why not just provide it out of the box like:

buttonRef: PropTypes.ref
joshalling commented 5 years ago

I would also like to see this feature added and would be willing to submit a PR for it if approved.

dzucconi commented 5 years ago
PropTypes.shape({ current: PropTypes.instanceOf(Element) })

Breaks under server-side rendering because of Element not existing in that context.

ReferenceError: Element is not defined

(I agree there should be a prop-type for refs.)

ahuth commented 5 years ago

One thing to note is that refs don't necessarily ref to a DOM element. This became especially clear while learning hooks. It's convenient to use refs to refer to callback functions, timeout ids, the current time, etc, while using useEffect, useMemo, useCallback, etc.

Generally I think of refs as being immutable references to mutable values.

Of course, maybe those kinds of refs aren't as likely to get passed around as props 🤔

ljharb commented 5 years ago

That seems less like a ref then, and more like state?

mindtraveller commented 5 years ago

@ahuth @ljharb The original idea was to create a prop-type to combine legacy callback refs and new object refs. Of course a separate prop-type for mutable object might also be added e.g. PropTypes.shape({ current: PropTypes.any }), because that structure has been widely used since useRef appeared.

oliviertassinari commented 5 years ago

I have one doubt with: PropTypes.instanceOf(Element). Does it work cross-window? cc @ryancogswell http://tobyho.com/2011/01/28/checking-types-in-javascript/.

react-popout can be a way to test that out.

ljharb commented 5 years ago

@oliviertassinari no, instanceof itself is broken and unreliable in this way, so PropTypes.instanceOf would have to be as well.

Izhaki commented 5 years ago

As @ahuth mentioned, we use refs for all sort of non-element things.

From the React Docs:

However, useRef() is useful for more than the ref attribute. It’s handy for keeping any mutable value around similar to how you’d use instance fields in classes.

In more than one occasion, whilst migrating class to functional component, I've found some reliance on a mutable value in the class component - and only refs can do this in functional components.

So I think instead of:

trimWidth: PropTypes.ref

it should be:

trimWidth: PropTypes.ref(PropTypes.number)