Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
ForwardRef supports propTypes
  • Loading branch information
Brian Vaughn committed May 25, 2018
commit 26e098faebaf8014623b0e352033d0302e5f4c9f
43 changes: 32 additions & 11 deletions packages/react/src/ReactElementValidator.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,11 @@ import lowPriorityWarning from 'shared/lowPriorityWarning';
import describeComponentFrame from 'shared/describeComponentFrame';
import isValidElementType from 'shared/isValidElementType';
import getComponentName from 'shared/getComponentName';
import {getIteratorFn, REACT_FRAGMENT_TYPE} from 'shared/ReactSymbols';
import {
getIteratorFn,
REACT_FORWARD_REF_TYPE,
REACT_FRAGMENT_TYPE,
} from 'shared/ReactSymbols';
import checkPropTypes from 'prop-types/checkPropTypes';
import warning from 'fbjs/lib/warning';

Expand Down Expand Up @@ -44,6 +48,14 @@ if (__DEV__) {
return element.type;
} else if (element.type === REACT_FRAGMENT_TYPE) {
return 'React.Fragment';
} else if (
typeof element.type === 'object' &&
element.type !== null &&
element.type.$$typeof === REACT_FORWARD_REF_TYPE
) {
const functionName =
element.type.render.displayName || element.type.render.name || '';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's hoist reading element.type so we don't keep repeating its reads?

return functionName !== '' ? `ForwardRef(${functionName})` : 'ForwardRef';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this look like getComponentName duplication here? Can we unify?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's similar. But getComponentName accepts a Fiber and this is a ReactElement.

We could refactor both to use a shared helper, but it didn't seem like the obvious correct thing to do in this case.

} else {
return element.type.displayName || element.type.name || 'Unknown';
}
Expand Down Expand Up @@ -213,30 +225,39 @@ function validateChildKeys(node, parentType) {
* @param {ReactElement} element
*/
function validatePropTypes(element) {
const componentClass = element.type;
if (typeof componentClass !== 'function') {
const type = element.type;
let name, propTypes;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be const propTypes = type.propTypes after the typeof type check, as that's the only value that it gets assigned?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current approach avoids reading propTypes from a possible null or undefined value (although that shouldn't actually happen in practice). I don't think it's a problem as-is.

if (typeof type === 'function') {
// Class or functional component
name = type.displayName || type.name;
propTypes = type.propTypes;
} else if (
typeof type === 'object' &&
type !== null &&
type.$$typeof === REACT_FORWARD_REF_TYPE
) {
// ForwardRef
const functionName = type.render.displayName || type.render.name || '';
name = functionName !== '' ? `ForwardRef(${functionName})` : 'ForwardRef';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, I feel like this is getting out of hand a little bit. We're going to forget updating some of these.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getting out of hand a bit

😝

We often inline things rather than reuse them. I made a judgement call. I'm happy to change it if you think I should.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Write everything twice. This is three times. :-)
But I don't care strongly.

propTypes = type.propTypes;
} else {
return;
}
const name = componentClass.displayName || componentClass.name;
const propTypes = componentClass.propTypes;
if (propTypes) {
currentlyValidatingElement = element;
checkPropTypes(propTypes, element.props, 'prop', name, getStackAddendum);
currentlyValidatingElement = null;
} else if (
componentClass.PropTypes !== undefined &&
!propTypesMisspellWarningShown
) {
} else if (type.PropTypes !== undefined && !propTypesMisspellWarningShown) {
propTypesMisspellWarningShown = true;
warning(
false,
'Component %s declared `PropTypes` instead of `propTypes`. Did you misspell the property assignment?',
name || 'Unknown',
);
}
if (typeof componentClass.getDefaultProps === 'function') {
if (typeof type.getDefaultProps === 'function') {
warning(
componentClass.getDefaultProps.isReactClassApproved,
type.getDefaultProps.isReactClassApproved,
'getDefaultProps is only used on classic React.createClass ' +
'definitions. Use a static property named `defaultProps` instead.',
);
Expand Down
47 changes: 47 additions & 0 deletions packages/react/src/__tests__/forwardRef-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,13 @@
'use strict';

describe('forwardRef', () => {
let PropTypes;
let React;
let ReactNoop;

beforeEach(() => {
jest.resetModules();
PropTypes = require('prop-types');
React = require('react');
ReactNoop = require('react-noop-renderer');
});
Expand Down Expand Up @@ -70,6 +72,51 @@ describe('forwardRef', () => {
expect(ref.current).toBe(null);
});

it('should support propTypes and defaultProps', () => {
function FunctionalComponent({forwardedRef, optional, required}) {
return (
<div ref={forwardedRef}>
{optional}
{required}
</div>
);
}

const RefForwardingComponent = React.forwardRef(function NamedFunction(
props,
ref,
) {
return <FunctionalComponent {...props} forwardedRef={ref} />;
});
RefForwardingComponent.propTypes = {
optional: PropTypes.string,
required: PropTypes.string.isRequired,
};
RefForwardingComponent.defaultProps = {
optional: 'default',
};

const ref = React.createRef();

ReactNoop.render(
<RefForwardingComponent ref={ref} optional="foo" required="bar" />,
);
ReactNoop.flush();
expect(ref.current.children).toEqual([{text: 'foo'}, {text: 'bar'}]);

ReactNoop.render(<RefForwardingComponent ref={ref} required="foo" />);
ReactNoop.flush();
expect(ref.current.children).toEqual([{text: 'default'}, {text: 'foo'}]);

expect(() =>
ReactNoop.render(<RefForwardingComponent ref={ref} optional="foo" />),
).toWarnDev(
'Warning: Failed prop type: The prop `required` is marked as required in ' +
'`ForwardRef(NamedFunction)`, but its value is `undefined`.\n' +
' in ForwardRef(NamedFunction) (at **)',
);
});

it('should warn if not provided a callback during creation', () => {
expect(() => React.forwardRef(undefined)).toWarnDev(
'forwardRef requires a render function but was given undefined.',
Expand Down