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 DEV warning for prop-types on render function #12644

Merged
merged 1 commit into from Apr 18, 2018

Conversation

Projects
None yet
7 participants
@bvaughn
Copy link
Contributor

bvaughn commented Apr 18, 2018

Someone at FB recently reported a bug that boiled down to the following:

function MyComponent(props, ref) {
  return <p ref={ref}>myProp: {props.myProp}</p>;
}
MyComponent.defaultProps = {
  myProp: "default"
};
MyComponent.propTypes = {
  myProp: React.PropTypes.string.isRequired
};

const MyWrappedComponent = React.forwardRef(MyComponent);

This kind of worked but, of course, prop-types didn't work.

What they meant to do was:

function MyComponent(props) {
  return <p ref={props.forwardedRef}>myProp: {props.myProp}</p>;
}
MyComponent.defaultProps = {
  myProp: "default"
};
MyComponent.propTypes = {
  myProp: React.PropTypes.string.isRequired
};

const MyWrappedComponent = React.forwardRef((props, ref) => (
  <MyComponent {...props} forwardedRef={ref} />
));

I think it would be nice for us to warn about this in DEV mode. I'm not enamored with this warning message in particular though if someone has better suggestions.

@pull-bot

This comment has been minimized.

Copy link

pull-bot commented Apr 18, 2018

Details of bundled changes.

Comparing: 0887c7d...4a64257

react

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react.development.js +0.4% +0.4% 55.22 KB 55.46 KB 15.17 KB 15.24 KB UMD_DEV
react.development.js +0.5% +0.5% 45.86 KB 46.1 KB 12.82 KB 12.88 KB NODE_DEV
React-dev.js +0.7% +0.7% 45.8 KB 46.11 KB 12.48 KB 12.56 KB FB_WWW_DEV

Generated by 🚫 dangerJS

@bvaughn bvaughn merged commit 920f30e into facebook:master Apr 18, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@bvaughn bvaughn deleted the bvaughn:forwardRef-propTypes-warning branch Apr 18, 2018

@eemeli

This comment has been minimized.

Copy link

eemeli commented May 23, 2018

I encountered this same issue, and was surprised that defaultProps and propTypes did not work for a forwardRef-wrapped render function.

@bvaughn Is there a reason why the defaultProps and propTypes could not be reassigned to the resulting component?

@bvaughn

This comment has been minimized.

Copy link
Contributor

bvaughn commented May 23, 2018

Hopefully the recently added warning will help with this 😄

There's a couple of reasons why defaultProps and propTypes aren't automatically forwarded:

The thing being returned might not be a functional or class component. It could be e.g. a <div> or a <Context.Consumer> etc.

The author of the forwardRef component necessarily has a reference to the underlying component anyway (MyComponent in the above example) and so can set propTypes or defaultProps directly. External users of a component shouldn't really be setting those, anymore than you would expect to override the propTypes value for any other third party component you use.

bors bot added a commit to mythmon/corsica-tree-status that referenced this pull request May 24, 2018

Merge #20
20: Update react monorepo to v16.4.0 r=renovate[bot] a=renovate[bot]

This Pull Request renovates the package group "react monorepo".


-   [react-dom](https://github.com/facebook/react) (`dependencies`): from `16.3.2` to `16.4.0`
-   [react](https://github.com/facebook/react) (`dependencies`): from `16.3.2` to `16.4.0`

# Release Notes
<details>
<summary>facebook/react</summary>

### [`v16.4.0`](https://github.com/facebook/react/blob/master/CHANGELOG.md#&#8203;1640-May-23-2018)
[Compare Source](facebook/react@8e5f12c...v16.4.0)
##### React

* Add a new [experimental](`reactjs/rfcs#51) `React.unstable_Profiler` component for measuring performance. ([@&#8203;bvaughn] in [#&#8203;12745](`facebook/react#12745))
##### React DOM

* Add support for the Pointer Events specification. ([@&#8203;philipp-spiess] in [#&#8203;12507](`facebook/react#12507))
* Properly call `getDerivedStateFromProps()` regardless of the reason for re-rendering. ([@&#8203;acdlite] in [#&#8203;12600](`facebook/react#12600) and [#&#8203;12802](`facebook/react#12802))
* Fix a bug that prevented context propagation in some cases. ([@&#8203;gaearon] in [#&#8203;12708](`facebook/react#12708))
* Fix re-rendering of components using `forwardRef()` on a deeper `setState()`. ([@&#8203;gaearon] in [#&#8203;12690](`facebook/react#12690))
* Fix some attributes incorrectly getting removed from custom element nodes. ([@&#8203;airamrguez] in [#&#8203;12702](`facebook/react#12702))
* Fix context providers to not bail out on children if there's a legacy context provider above. ([@&#8203;gaearon] in [#&#8203;12586](`facebook/react#12586))
* Add the ability to specify `propTypes` on a context provider component. ([@&#8203;nicolevy] in [#&#8203;12658](`facebook/react#12658))
* Fix a false positive warning when using `react-lifecycles-compat` in `<StrictMode>`. ([@&#8203;bvaughn] in [#&#8203;12644](`facebook/react#12644))
* Warn when the `forwardRef()` render function has `propTypes` or `defaultProps`. ([@&#8203;bvaughn] in [#&#8203;12644](`facebook/react#12644))
* Improve how `forwardRef()` and context consumers are displayed in the component stack. ([@&#8203;sophiebits] in [#&#8203;12777](`facebook/react#12777))
* Change internal event names. This can break third-party packages that rely on React internals in unsupported ways. ([@&#8203;philipp-spiess] in [#&#8203;12629](`facebook/react#12629))
##### React Test Renderer

* Fix the `getDerivedStateFromProps()` support to match the new React DOM behavior. ([@&#8203;koba04] in [#&#8203;12676](`facebook/react#12676))
* Fix a `testInstance.parent` crash when the parent is a fragment or another special node. ([@&#8203;gaearon] in [#&#8203;12813](`facebook/react#12813))
* `forwardRef()` components are now discoverable by the test renderer traversal methods. ([@&#8203;gaearon] in [#&#8203;12725](`facebook/react#12725))
* Shallow renderer now ignores `setState()` updaters that return `null` or `undefined`. ([@&#8203;koba04] in [#&#8203;12756](`facebook/react#12756))
##### React ART

* Fix reading context provided from the tree managed by React DOM. ([@&#8203;acdlite] in [#&#8203;12779](`facebook/react#12779))
##### React Call Return (Experimental)

* This experiment was deleted because it was affecting the bundle size and the API wasn't good enough. It's likely to come back in the future in some other form. ([@&#8203;gaearon] in [#&#8203;12820](`facebook/react#12820))
##### React Reconciler (Experimental)

* The [new host config shape](https://github.com/facebook/react/blob/c601f7a64640290af85c9f0e33c78480656b46bc/packages/react-noop-renderer/src/createReactNoop.js#L82-L285) is flat and doesn't use nested objects. ([@&#8203;gaearon] in [#&#8203;12792](`facebook/react#12792))

---


</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).

Co-authored-by: Renovate Bot <bot@renovateapp.com>

bors bot added a commit to mozilla/delivery-console that referenced this pull request May 24, 2018

Merge #164
164: Update react monorepo to v16.4.0 r=rehandalal a=renovate[bot]

This Pull Request renovates the package group "react monorepo".


-   [react-dom](https://github.com/facebook/react) (`dependencies`): from `16.3.2` to `16.4.0`
-   [react](https://github.com/facebook/react) (`dependencies`): from `16.3.2` to `16.4.0`

# Release Notes
<details>
<summary>facebook/react</summary>

### [`v16.4.0`](https://github.com/facebook/react/blob/master/CHANGELOG.md#&#8203;1640-May-23-2018)
[Compare Source](facebook/react@8e5f12c...v16.4.0)
##### React

* Add a new [experimental](`reactjs/rfcs#51) `React.unstable_Profiler` component for measuring performance. ([@&#8203;bvaughn] in [#&#8203;12745](`facebook/react#12745))
##### React DOM

* Add support for the Pointer Events specification. ([@&#8203;philipp-spiess] in [#&#8203;12507](`facebook/react#12507))
* Properly call `getDerivedStateFromProps()` regardless of the reason for re-rendering. ([@&#8203;acdlite] in [#&#8203;12600](`facebook/react#12600) and [#&#8203;12802](`facebook/react#12802))
* Fix a bug that prevented context propagation in some cases. ([@&#8203;gaearon] in [#&#8203;12708](`facebook/react#12708))
* Fix re-rendering of components using `forwardRef()` on a deeper `setState()`. ([@&#8203;gaearon] in [#&#8203;12690](`facebook/react#12690))
* Fix some attributes incorrectly getting removed from custom element nodes. ([@&#8203;airamrguez] in [#&#8203;12702](`facebook/react#12702))
* Fix context providers to not bail out on children if there's a legacy context provider above. ([@&#8203;gaearon] in [#&#8203;12586](`facebook/react#12586))
* Add the ability to specify `propTypes` on a context provider component. ([@&#8203;nicolevy] in [#&#8203;12658](`facebook/react#12658))
* Fix a false positive warning when using `react-lifecycles-compat` in `<StrictMode>`. ([@&#8203;bvaughn] in [#&#8203;12644](`facebook/react#12644))
* Warn when the `forwardRef()` render function has `propTypes` or `defaultProps`. ([@&#8203;bvaughn] in [#&#8203;12644](`facebook/react#12644))
* Improve how `forwardRef()` and context consumers are displayed in the component stack. ([@&#8203;sophiebits] in [#&#8203;12777](`facebook/react#12777))
* Change internal event names. This can break third-party packages that rely on React internals in unsupported ways. ([@&#8203;philipp-spiess] in [#&#8203;12629](`facebook/react#12629))
##### React Test Renderer

* Fix the `getDerivedStateFromProps()` support to match the new React DOM behavior. ([@&#8203;koba04] in [#&#8203;12676](`facebook/react#12676))
* Fix a `testInstance.parent` crash when the parent is a fragment or another special node. ([@&#8203;gaearon] in [#&#8203;12813](`facebook/react#12813))
* `forwardRef()` components are now discoverable by the test renderer traversal methods. ([@&#8203;gaearon] in [#&#8203;12725](`facebook/react#12725))
* Shallow renderer now ignores `setState()` updaters that return `null` or `undefined`. ([@&#8203;koba04] in [#&#8203;12756](`facebook/react#12756))
##### React ART

* Fix reading context provided from the tree managed by React DOM. ([@&#8203;acdlite] in [#&#8203;12779](`facebook/react#12779))
##### React Call Return (Experimental)

* This experiment was deleted because it was affecting the bundle size and the API wasn't good enough. It's likely to come back in the future in some other form. ([@&#8203;gaearon] in [#&#8203;12820](`facebook/react#12820))
##### React Reconciler (Experimental)

* The [new host config shape](https://github.com/facebook/react/blob/c601f7a64640290af85c9f0e33c78480656b46bc/packages/react-noop-renderer/src/createReactNoop.js#L82-L285) is flat and doesn't use nested objects. ([@&#8203;gaearon] in [#&#8203;12792](`facebook/react#12792))

---


</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).

Co-authored-by: Renovate Bot <bot@renovateapp.com>
@eemeli

This comment has been minimized.

Copy link

eemeli commented May 24, 2018

I don't think the argument regarding the eventual result of the render applies here, and I'm pretty sure that its defaultProps and propTypes could both be applied to the object returned by forwardRef. Manually doing so in fact already works for defaultProps, but for propTypes validatePropTypes(element) in ReactElementValidator requires that element.type is a function.

I do agree that there is an alternative formulation available here for the component author that does allow for the desired result, but I have two concerns with the current situation:

  • It feels clumsy to have to create an intermediary component just to get ref into the inner props object with some other name.
  • The shape of the forwardRef argument (props, ref) => node makes it look very much like a functional component, and it's not intuitive that it then works as a functional component in all other aspects except for how its defaultProps and propTypes work.

Would you be open for a PR fixing this, or are there murkier depths that I'm not considering here?

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented May 24, 2018

but for propTypes validatePropTypes(element) in ReactElementValidator requires that element.type is a function.

This sounds like an omission to me. Let’s fix that?

@bvaughn

This comment has been minimized.

Copy link
Contributor

bvaughn commented May 24, 2018

I'm pretty sure that its defaultProps and propTypes could both be applied to the object returned by forwardRef.

Maybe we're saying different things? I'll attempt to clarify.

This works. (Prop type warnings will fire for InnerComponent):

function InnerComponent(props) { /* ... */ }
InnerComponent.propTypes = {
  foo: PropTypes.string.isRequired
};

const OuterComponent = React.forwardRef(
  (props, ref) => <InnerComponent ref={ref} {...props} />
);

This is intentionally not supported:

function InnerComponent(props) { /* ... */ }

const OuterComponent = React.forwardRef(
  (props, ref) => <InnerComponent ref={ref} {...props} />
);
OuterComponent.propTypes = {
  foo: PropTypes.string.isRequired
};

External code shouldn't be overriding propTypes or defaultProps. For example, you wouldn't do this:

import { Link } from 'react-router-dom';

Link.propTypes = { /* ... */ };

Attributes like propTypes and defaultProps are part of the component. They should be declared where the component is declared. (And the place where forwardRef is used will necessarily have access to the original component, in order to actually return it.)

It feels clumsy to have to create an intermediary component just to get ref into the inner props object with some other name.

This is required in order to not break backwards compatibility. I know reading old RFCs is a lot of work, but the necessity of this was considered and discussed in the ref forwarding RFC. (You might be interested in checking out that portion of the RFC at least! 😄)

@eemeli

This comment has been minimized.

Copy link

eemeli commented May 25, 2018

Yeah, I think we're talking about slightly different things. What I would like to work is this (note the two args of Componentish):

const Componentish = (props, ref) => (<div {...props} ref={ref} />)
Componentish.propTypes = { foo: PropTypes.string.isRequired, bar: PropTypes.string }
Componentish.defaultProps = { bar: 'BAZ' }
export default React.forwardRef(Componentish);

At the moment, that produces the warning of this PR, but as a developer I'd prefer if instead it "just worked", which I believe would be possible with something like this backwards-compatible solution (not meant to be production code, just a sketch of what I mean):

--- a/packages/react/src/ReactElement.js
+++ b/packages/react/src/ReactElement.js
@@ -219,8 +219,8 @@ export function createElement(type, config, children) {
   }

   // Resolve default props
-  if (type && type.defaultProps) {
-    const defaultProps = type.defaultProps;
+  const defaultProps = type && (type.defaultProps || (type.render && type.render.defaultProps));
+  if (defaultProps) {
     for (propName in defaultProps) {
       if (props[propName] === undefined) {
         props[propName] = defaultProps[propName];

--- a/packages/react/src/ReactElementValidator.js
+++ b/packages/react/src/ReactElementValidator.js
@@ -213,11 +213,19 @@ function validateChildKeys(node, parentType) {
  * @param {ReactElement} element
  */
 function validatePropTypes(element) {
-  const componentClass = element.type;
+  let componentClass = element.type;
   if (typeof componentClass !== 'function') {
-    return;
+    if (
+      typeof componentClass === 'object' &&
+      componentClass !== null &&
+      typeof componentClass.render === 'function'
+    ) {
+      componentClass = componentClass.render;
+    } else {
+      return;
+    }
   }
-  const name = componentClass.displayName || componentClass.name;
+  const name = getComponentName(element)
   const propTypes = componentClass.propTypes;
   if (propTypes) {
     currentlyValidatingElement = element;

I have read through the RFC discussion, and I don't think that this option was considered. Now, whether it's an option that could be considered is something I think you might be more qualified to answer. :)

@bvaughn

This comment has been minimized.

Copy link
Contributor

bvaughn commented May 25, 2018

I see. Thank you for clarifying. 😄

It still seems to me like you should be able to accomplish what you're trying to do like this:

const Component = ({ forwardedRef, ...props }) => <div {...props} ref={ref} />;
Component.propTypes = {
  foo: PropTypes.string.isRequired,
  bar: PropTypes.string
};
Component.defaultProps = { bar: "BAZ" };

export default React.forwardRef((props, ref) => (
  <Component {...props} forwardedRef={ref} />
));

Is there a reason this doesn't work? (Why do the propTypes and defaultProps need to be attached to the ref-forwarding function? Just because it's more succinct?)

@piuccio

This comment has been minimized.

Copy link

piuccio commented May 29, 2018

guys can you maybe also update this page, please? https://reactjs.org/docs/forwarding-refs.html

I've read this conversation and still I'm not quite sure where I'm now supposed to put my prop-types.

All I know is that the following code gives a warning in 16.4.0

function Component(props, ref) {}
Component.propTypes = {};
export default React.forwardRef(Component);

I have no clue on how to fix it, but I'll wait for #12911, maybe it'll go away magically as it appeared.

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented May 29, 2018

@piuccio The thing you put into forwardRef is not a "component". It's a render function.

This should work:

function MyComponent(props) {}
MyComponent.propTypes = {};
export default React.forwardRef((props, ref) => <MyComponent {...props} forwardedRef={ref} />);

In the next release we'll make this work too:

function renderComponent(props, ref) {}
const MyComponent = React.forwardRef(renderComponent);
MyComponent.propTypes = {};
export default MyComponent;

(But it doesn't work yet.)

@dependencies dependencies bot referenced this pull request May 29, 2018

Closed

Update yarn.lock #372

@bvaughn

This comment has been minimized.

Copy link
Contributor

bvaughn commented May 29, 2018

@piuccio The thing you put into forwardRef is not a "component". It's a render function.

Just wanted to add that the warning will remain for this case too, since it will continue to not work 😄

Just to recap:

function MyComponent(props) {
  // ...
}
MyComponent.propTypes = {}; // This will work now

function renderFunction(props, ref) {
  return <MyComponent {...props} forwardedRef={ref} />;
}
renderFunction.propTypes = {}; // This WILL NOT work

const ExportedComponent = React.forwardRef(renderFunction);
ExportedComponent.propTypes = {}; // This will work in the next release
@piuccio

This comment has been minimized.

Copy link

piuccio commented May 30, 2018

Thanks, I've fixed my code. It would also be nice to mention in the documentation that forwardedRef is a PropTypes.object

@bvaughn

This comment has been minimized.

Copy link
Contributor

bvaughn commented May 30, 2018

Thanks, I've fixed my code. It would also be nice to mention in the documentation that forwardedRef is a PropTypes.object

It's not. It is an Object with a couple of special keys that React knows how to recognize and handle. It isn't really meant to be used for anything other than passing to React.createElement, so its type/shape didn't seem too important to document in detail.

@piuccio

This comment has been minimized.

Copy link

piuccio commented May 30, 2018

Well, if you use eslint-plugin-react to lint your code it'll complain if you don't put forwardedRef inside .propTypes and of all the types, the only one that doesn't generate a warning during development is PropTypes.object. I guess PropTypes.shape would work as well, but it's more typing to also specify those special keys.

So for plain users like me, I guess PropTypes.object is the best guess.

Even if the shape is not too important, having it in documentation will help people understand where to defined the .propTypes

@bvaughn

This comment has been minimized.

Copy link
Contributor

bvaughn commented May 30, 2018

@piuccio I misread your original message as "forwardRef" rather than "forwardedRef". Sorry for any confusion my response caused.

That being said, I'm not quite sure what you're saying. The forwardedRef prop would just be a regular prop, like any other, except this one happens to contain a ref. That particular pattern has been around for a long time- although the naming varies from project to project.

If you want to specify it as one of the prop-types for the inner component, I would suggest using PropTypes.any. 😄

So maybe something like this would work for you, at a high level?

const sharedPropTypes = {
  bar: PropType.string.isRequired,
  foo: PropType.string.isRequired
};

const SomeComponent = ({ bar, foo, forwardedRef }) => {
  // Your component here...
};
SomeComponent.propTypes = {
  ...sharedPropTypes,
  forwardedRef: PropTypes.any
};

const RefForwardingWrapper = React.forwardRef((props, ref) => (
  <SomeComponent {...props} forwardedRef={ref} />
));
RefForwardingWrapper.propTypes = sharedPropTypes;
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment