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 forwardRef / valid component prop type #200

Closed
aldeed opened this issue Jul 31, 2018 · 18 comments
Closed

Add forwardRef / valid component prop type #200

aldeed opened this issue Jul 31, 2018 · 18 comments

Comments

@aldeed
Copy link

aldeed commented Jul 31, 2018

A component expecting another component to be passed in can do something like this:

Button: PropTypes.oneOfType([PropTypes.string, PropTypes.func]).isRequired

However, if I pass in a Button component that has been wrapped in forwardRef, the prop type check fails. forwardRef returns an object with render function, not a function.

Can there be a built-in prop type for the forwardRef object?

And/or maybe a prop type for "anything that works as a component", which would be func, forwardRef obj, and any strings recognized as built-in DOM components.

See https://stackoverflow.com/questions/45315918/react-proptypes-component-class/45316411, which suggests:

FancyButton: function (props, propName, componentName) {
    if(!props[propName] || typeof(props[propName].render) != 'function') {
      return new Error(`${propName}.render must be a function!`);
    }
  },
@ljharb
Copy link
Collaborator

ljharb commented Jul 31, 2018

I think a Component propType would be better - that way as further types are created, it can be expanded to allow them (like forward refs)

@aldeed
Copy link
Author

aldeed commented Jul 31, 2018

Agree @ljharb. For reference, this is the custom prop type I made that is working:

import { isValidElementType } from "react-is";

/**
 * We use an Error-like object for backward compatibility as people may call
 * PropTypes directly and inspect their output. However, we don't use real
 * Errors anymore. We don't inspect their stack anyway, and creating them
 * is prohibitively expensive if they are created too often, such as what
 * happens in oneOfType() for any type before the one that matched.
 */
function PropTypeError(message) {
  this.message = message;
  this.stack = "";
}
// Make `instanceof Error` still work for returned errors.
PropTypeError.prototype = Error.prototype;

function getComponentValidator() {
  function checkType(isRequired, props, propName, componentName, location, propFullName) {
    const val = props[propName];
    if (val == null) {
      if (isRequired) {
        if (props[propName] === null) {
          return new PropTypeError(`The ${location} '${propFullName}' is marked as required in '${componentName}', but its value is 'null'.`);
        }
        return new PropTypeError(`The ${location} '${propFullName}' is marked as required in '${componentName}', but its value is 'undefined'.`);
      }
    } else if (!isValidElementType(val)) {
      return new PropTypeError(`Invalid ${location} '${propFullName}' supplied to ${componentName}. Expected a string (for built-in components) or a class/function (for composite components).`);
    }
    return null;
  }

  const chainedCheckType = checkType.bind(null, false);
  chainedCheckType.isRequired = checkType.bind(null, true);
  return chainedCheckType;
}

const CustomPropTypes = {
  component: getComponentValidator(),
};

export default CustomPropTypes;

Usage:

import CustomPropTypes from "./CustomPropTypes";

FancyButton: CustomPropTypes.component.isRequired

Possibly it should be called validElementType to match the naming in the react-is package?

@aldeed aldeed changed the title Add forwardRef prop type Add forwardRef / valid component prop type Jul 31, 2018
@ljharb
Copy link
Collaborator

ljharb commented Aug 1, 2018

This is a component, not an element?

For elements, there’s already an element propType - that should theoretically already work for forward refs.

@ljharb
Copy link
Collaborator

ljharb commented Aug 1, 2018

(Or if there’s not, there should be both component and element - am mobile rn so i can’t check)

@aldeed
Copy link
Author

aldeed commented Aug 1, 2018

Yes, a component. forwardRef doesn't return an element, and we don't want to allow elements, only anything that can be used as a "component". So anything that passes the isValidElementType check.

kachkaev added a commit to kachkaev/react-highlighter that referenced this issue Sep 11, 2018
This is a temporary solution, see facebook/prop-types#200 for details.
lucarge added a commit to lucarge/react-router that referenced this issue Nov 2, 2018
Hi all,
[since six months](facebook/react-native@e708010) the `react-native` implementation of `Text` implemented `forwardRef`. This means that, while a `Text` component is legit as a `Link` (it implements the `onPress` prop), it's not a function anymore. Instead, it will be an object with a render function.

Unfortunately, the right prop type for `forwardRef` [does not exists yet](facebook/prop-types#200).

This PR allows `Text` to be passed as `component` to `Link`, without `prop-types` warnings and maintaining a good level of safety checks.

I hope you'll be ok with merging this, and thanks again for `react-router` 🙌
@wmertens
Copy link

wmertens commented Dec 4, 2018

👍 specifically for adding a component PropType

@dfee
Copy link

dfee commented Dec 16, 2018

Here is my current solution:

import PropTypes from "prop-types";
import React from "react";

describe("propTypes", () => {
  let error: typeof global.console.error; // typescript (feel free to remove)

  beforeEach(() => {
    error = global.console.error;
    global.console.error = jest.fn();
  });

  afterEach(() => {
    global.console.error = error;
  });

  it("should not warn on node [forwardRef]", () => {
    const propTypes = {
      as: PropTypes.oneOfType([
        PropTypes.func,
        PropTypes.string,
        PropTypes.shape({ render: PropTypes.func.isRequired }),
      ]),
    };
    const as = React.forwardRef((props, ref) =>
      React.createElement("div", { ref, ...props }),
    );
    PropTypes.checkPropTypes(propTypes, { as }, "prop", "test");
    expect(errorMock.mock.calls).toHaveLength(0);
  });
});

@wmertens
Copy link

wmertens commented Dec 17, 2018

@dfee thanks! I polyfilled prop-types with it:

polyfills.js:

import PropTypes from 'prop-types'

PropTypes.component = PropTypes.oneOfType([
	PropTypes.func,
	PropTypes.string,
	PropTypes.shape({render: PropTypes.func.isRequired}),
])

and now I put PropTypes.component wherever necessary

@ljharb
Copy link
Collaborator

ljharb commented Dec 17, 2018

"it has a render function" is a pretty brittle check, when react-is has a robust isForwardRef method available.

@wmertens
Copy link

@ljharb sure, but there aren't all that many objects with render functions that are also not a component. In my case it's a quick fix which helps tremendously. It would be great if prop-types added this, in the proper version with react-is. Is that on the table?

@eps1lon
Copy link

eps1lon commented Jan 13, 2019

"it has a render function" is a pretty brittle check, when react-is has a robust isForwardRef method available.

react-is has a check for forwardRef elements not components. For a component propType all you need is isValidElementType.

You could use import { componentPropType } from '@material-ui/utils'. This works very similar to validators in the ´prop-typespackage and validates given values by usingisValidElementType`.

bpierre added a commit to aragon/ui that referenced this issue Jan 27, 2019
Prevents TableCell to emit a warning when a styled component v4+ is
passed (see related issue comments [1][2]).

[1] styled-components/styled-components#2271 (comment)
[2] facebook/prop-types#200 (comment)
bpierre added a commit to aragon/ui that referenced this issue Jan 27, 2019
Prevents TableCell to emit a warning when a styled component v4+ is
passed (see related issue comments [1][2]).

[1] styled-components/styled-components#2271 (comment)
[2] facebook/prop-types#200 (comment)
@ljharb
Copy link
Collaborator

ljharb commented Feb 11, 2019

elementType now exists.

@ljharb ljharb closed this as completed Feb 11, 2019
@wmertens
Copy link

@ljharb I'm confused by that naming - I though React elements were the instantiated objects and components the classes/functions/strings that instantiate them?

@ljharb
Copy link
Collaborator

ljharb commented Feb 11, 2019

Ah, my mistake - this issue is looking for componentType, not elementType. Reopening.

@ljharb ljharb reopened this Feb 11, 2019
@wmertens
Copy link

Looking at b67bbd4, it seems that .elementType is exactly the .component we're looking for. Just the naming is odd.

@ljharb ljharb closed this as completed Feb 11, 2019
@ljharb
Copy link
Collaborator

ljharb commented Feb 11, 2019

div isn't a component, i suppose, so elementType makes the most sense?

@eps1lon
Copy link

eps1lon commented Feb 11, 2019

It matches the terminology react is using internally. <Component /> is transpiled to React.createElement(Component) and React.createElement takes an element type as its first argument.

@wmertens
Copy link

Hmmm, indeed, https://reactjs.org/docs/react-api.html#createelement makes a distinction between components, fragments and strings, but all are element types. Name is ok for me, I don't need a test for just components 👍

lonyele added a commit to lonyele/reactjs.org that referenced this issue Jul 13, 2019
Hi, I've met the same issue on [here](facebook/prop-types#200) at storybook and found the following [PR](facebook/prop-types#211) that adds the `elementType` feature. It could find the doc on npm, but not the official react site.
lex111 pushed a commit to reactjs/react.dev that referenced this issue Jul 13, 2019
Hi, I've met the same issue on [here](facebook/prop-types#200) at storybook and found the following [PR](facebook/prop-types#211) that adds the `elementType` feature. It could find the doc on npm, but not the official react site.
another-guy pushed a commit to reactjs/ru.react.dev that referenced this issue Jul 30, 2019
* Update thinking-in-react.md (#2095)

Please refer to https://justsimply.dev for the thinking behind these proposed changes.

* Update thinking-in-react.md (#2098)

Follow up to reactjs/react.dev#2095 (comment)

* Add missing function call to example (#2102)

An example for useEffect omitted the actual invocation of the function in question.

* Add description of PropTypes.exact (#1581)

Info from https://github.com/facebook/prop-types#usage

* Improve grammar (#2106)

* Fixed minor code-splitting.md typo (#1292)

* Fixed minor code-splitting.md typo

* Update code-splitting.md


Co-authored-by: Alexey Pyltsyn <lex61rus@gmail.com>

* Fixed broken link to discuss.react.org (#2107)

* Replaced broken discuss.reactjs.org link

On the how-to-contribute page, there is a broken link under the https://reactjs.org/docs/how-to-contribute.html#how-to-get-in-touch section. As outlined in reactjs/react.dev#2080 `discuss.reactjs.org` isn't reachable.

I edited the text to display `Discussion Forums` which links to https://reactjs.org/community/support.html#popular-discussion-forums (as I was unable to find an official reactjs discussion forum).

* fixed text case

changed `Discussion Forums` to `Discussion forums`

* Update 2019-02-23-is-react-translated-yet.md (#2109)

* Add Meetup (#2097)

Add JaipurJS - JavaScript meetup in Jaipur, Rajasthan, India

* [docs] Updated required node and npm versions to match CRA docs in 'docs/create-a-new-react-app.html' (#2099)

https://facebook.github.io/create-react-app/docs/getting-started

* Remove tooling support info in fragment docs (#2096)

* Correct the description of when gDSFP gets called (#2100)

* Added free Scrimba React Tutorial (#2111)

A great video/editor tutorial consisting of 48 hands-on lessons.

* Update Production Optimization docs to use terser (#2112)

* Update Production Optimization docs to use terser

* Update Optimizing Performance.md

* Fix typo

Co-Authored-By: Alexey Pyltsyn <lex61rus@gmail.com>

* Update hooks-faq.md (#2113)

* Update hooks-faq.md

I tripped up slightly while reading this example for using the callback form of a state setter inside an effect. I've added a few lines that might help a hook newbie grok the differences between the examples.

* Update hooks-faq.md

* Update hooks-faq.md

* Update tutorial.md (#2115)

changed 'any React apps' to 'any React app'

* move past conferences to the bottom of the list (#2118)

* fix(Blog): Post title updated to correct word for yes word in spanish (#2122)

* Revert "fix(Blog): Post title updated to correct word for yes word in spanish (#2122)" (#2130)

This reverts commit 06a029d.

* Add DevExtreme Reactive to the Components list (#2127)

* [Documentation] Fix: Update link to Chrome Accessibility Inspec… (#2134)

* React Native added support for hooks in 0.59 (#2121)

* React Native added support for hooks in 0.59

React Native 0.59 and above already support React Hooks, this line is no longer necessary, causes confusion for some people that it is not working right now. We can also mention React Native version if needed.

* update with react native mention of hooks support

* Update content/docs/hooks-faq.md

suggested changes

Co-Authored-By: Alexey Pyltsyn <lex61rus@gmail.com>

* Add Kiel to the list of React Meetups (#2136)

* Reduce confusion about adding additional fields to .this (#2142)

As a new React learner, this part was a bit confusing as I though that it was referencing `() => this.tick()` part of the code. My addition would help at least people like me.

* Added option for more cdns. (#2144)

* Update docs about an existence of .elementType (#2145)

Hi, I've met the same issue on [here](facebook/prop-types#200) at storybook and found the following [PR](facebook/prop-types#211) that adds the `elementType` feature. It could find the doc on npm, but not the official react site.

* Revert "Added option for more cdns. (#2144)" (#2146)

This reverts commit b84fb3d.

* Add React Conf to list of community conferences (#2158)

* Add React Conf to list of community conferences

* Whoops, put right day in

* docs(hooks): fix typo (#2161)

*  update the status of Arabic translation .. (#2157)

* Fixing typo in contributing section of the docs (#2166)

* Add a relevant FAQ link in "Thinking In React" (#2170)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants