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

[utils] Add support for forwardRef components in getDisplayName #14429

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
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
3 changes: 2 additions & 1 deletion .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ module.exports = {
'react/no-find-dom-node': 'off',

'jsx-a11y/no-autofocus': 'off', // We are a library, people do what they want.
'prefer-arrow-callback': ['error', { allowNamedFunctions: true }],
Copy link
Member Author

Choose a reason for hiding this comment

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

This is also required for react-dev-tools to generate a meaningful display name for forwardRef components. The correct exception would be to allow them only in forwardRef.

'prefer-destructuring': 'off', // Destructuring harm grep potential.
'consistent-this': ['error', 'self'],
'max-len': [
Expand Down Expand Up @@ -90,7 +91,7 @@ module.exports = {
'mocha/no-sibling-hooks': 'error',
'mocha/no-skipped-tests': 'error',
'mocha/no-top-level-hooks': 'error',
'mocha/prefer-arrow-callback': 'error',
'mocha/prefer-arrow-callback': ['error', { allowNamedFunctions: true }],
'mocha/valid-suite-description': 'error',
},
};
4 changes: 2 additions & 2 deletions .size-limit.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,13 @@ module.exports = [
name: 'The size of the @material-ui/core modules',
webpack: true,
path: 'packages/material-ui/build/index.js',
limit: '91.3 KB',
limit: '91.2 KB',
Copy link
Member Author

Choose a reason for hiding this comment

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

Saved most of the increases from #13722 and #14423.

},
{
name: 'The size of the @material-ui/styles modules',
webpack: true,
path: 'packages/material-ui-styles/build/index.js',
limit: '16.3 KB',
limit: '16.4 KB',
Copy link
Member Author

Choose a reason for hiding this comment

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

For some reason @material-ui/utils appears in the styles bundle. Tree shaking is working fine but the dependency is not removed in a production bundle. It doesn't appear in the core bundle though.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does not appear in the esm bundle. I would keep this for now and revisit what bundles we monitor for size later.

},
{
name: 'The size of the @material-ui/system modules',
Expand Down
45 changes: 41 additions & 4 deletions packages/material-ui-utils/src/getDisplayName.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Fork of recompose/getDisplayName with added IE 11 support
import { ForwardRef } from 'react-is';

// Simplified polyfill for IE 11 support
// https://github.com/JamesMGreene/Function.name/blob/58b314d4a983110c3682f1228f845d39ccca1817/Function.name.js#L3
Expand All @@ -9,16 +9,53 @@ export function getFunctionName(fn) {
return name || '';
}

/**
* @param {function} Component
* @param {string} fallback
* @returns {string | undefined}
*/
function getFunctionComponentName(Component, fallback = '') {
return Component.displayName || Component.name || getFunctionName(Component) || fallback;
}

function getWrappedName(outerType, innerType, wrapperName) {
const functionName = getFunctionComponentName(innerType);
return (
outerType.displayName || (functionName !== '' ? `${wrapperName}(${functionName})` : wrapperName)
);
}

/**
* cherry-pick from
* https://github.com/facebook/react/blob/769b1f270e1251d9dbdce0fcbd9e92e502d059b8/packages/shared/getComponentName.js
* originally forked from recompose/getDisplayName with added IE 11 support
*
* @param {React.ReactType} Component
* @returns {string | undefined}
*/
function getDisplayName(Component) {
if (Component == null) {
return undefined;
}

if (typeof Component === 'string') {
return Component;
}

if (!Component) {
return undefined;
if (typeof Component === 'function') {
return getFunctionComponentName(Component, 'Component');
}

if (typeof Component === 'object') {
switch (Component.$$typeof) {
case ForwardRef:
return getWrappedName(Component, Component.render, 'ForwardRef');
default:
return undefined;
}
}

return Component.displayName || Component.name || getFunctionName(Component) || 'Component';
return undefined;
}

export default getDisplayName;
16 changes: 16 additions & 0 deletions packages/material-ui-utils/src/getDisplayName.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,29 @@ describe('utils/getDisplayName.js', () => {

const AndAnotherComponent = () => <div />;

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

const ForwardRefComponent = React.forwardRef(function Div(props, ref) {
return <div {...props} ref={ref} />;
});

const NamedForwardRefComponent = React.forwardRef((props, ref) => (
<div {...props} ref={ref} />
));
NamedForwardRefComponent.displayName = 'Div';

assert.strictEqual(getDisplayName(SomeComponent), 'SomeComponent');
assert.strictEqual(getDisplayName(SomeOtherComponent), 'CustomDisplayName');
assert.strictEqual(getDisplayName(YetAnotherComponent), 'YetAnotherComponent');
assert.strictEqual(getDisplayName(AndAnotherComponent), 'AndAnotherComponent');
assert.strictEqual(getDisplayName(() => <div />), 'Component');
assert.strictEqual(getDisplayName('div'), 'div');
assert.strictEqual(getDisplayName(), undefined);
assert.strictEqual(getDisplayName(AnonymousForwardRefComponent), 'ForwardRef');
assert.strictEqual(getDisplayName(ForwardRefComponent), 'ForwardRef(Div)');
assert.strictEqual(getDisplayName(NamedForwardRefComponent), 'Div');
});
});

Expand Down
2 changes: 1 addition & 1 deletion packages/material-ui/scripts/rollup.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ const commonjsOptions = {
ignoreGlobal: true,
include: /node_modules/,
namedExports: {
'../../node_modules/react-is/index.js': ['isValidElementType'],
'../../node_modules/react-is/index.js': ['ForwardRef', 'isValidElementType'],
},
};

Expand Down
4 changes: 1 addition & 3 deletions packages/material-ui/src/Backdrop/Backdrop.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export const styles = {
},
};

const Backdrop = React.forwardRef((props, ref) => {
const Backdrop = React.forwardRef(function Backdrop(props, ref) {
const { classes, className, invisible, open, transitionDuration, ...other } = props;

return (
Expand All @@ -46,8 +46,6 @@ const Backdrop = React.forwardRef((props, ref) => {
);
});

Backdrop.displayName = 'Backdrop';

Backdrop.propTypes = {
/**
* Override or extend the styles applied to the component.
Expand Down
4 changes: 1 addition & 3 deletions packages/material-ui/src/List/List.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export const styles = {
},
};

const List = React.forwardRef((props, ref) => {
const List = React.forwardRef(function List(props, ref) {
const {
children,
classes,
Expand Down Expand Up @@ -63,8 +63,6 @@ const List = React.forwardRef((props, ref) => {
);
});

List.displayName = 'List';

List.propTypes = {
/**
* The content of the component.
Expand Down
4 changes: 1 addition & 3 deletions packages/material-ui/src/ListItem/ListItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ export const styles = theme => ({
/**
* Uses an additional container component if `ListItemSecondaryAction` is the last child.
*/
const ListItem = React.forwardRef((props, ref) => {
const ListItem = React.forwardRef(function ListItem(props, ref) {
const {
alignItems,
button,
Expand Down Expand Up @@ -177,8 +177,6 @@ const ListItem = React.forwardRef((props, ref) => {
);
});

ListItem.displayName = 'ListItem';

ListItem.propTypes = {
/**
* Defines the `align-items` style property.
Expand Down
4 changes: 1 addition & 3 deletions packages/material-ui/src/MenuItem/MenuItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export const styles = theme => ({
selected: {},
});

const MenuItem = React.forwardRef((props, ref) => {
const MenuItem = React.forwardRef(function MenuItem(props, ref) {
const { classes, className, component, disableGutters, role, selected, ...other } = props;

return (
Expand All @@ -52,8 +52,6 @@ const MenuItem = React.forwardRef((props, ref) => {
);
});

MenuItem.displayName = 'MenuItem';

MenuItem.propTypes = {
/**
* Menu item contents.
Expand Down
1 change: 0 additions & 1 deletion packages/material-ui/src/MenuList/MenuList.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ describe('<MenuList />', () => {
});

it('should render a List', () => {
assert.strictEqual(wrapper.name(), 'List');
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why you would test here for that specific component. Would ul be enough or some html element that matches those semantics?

assert.strictEqual(wrapper.props()['data-test'], 'hi');
assert.strictEqual(wrapper.hasClass('test-class'), true);
});
Expand Down
4 changes: 1 addition & 3 deletions packages/material-ui/src/Paper/Paper.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export const styles = theme => {
};
};

const Paper = React.forwardRef((props, ref) => {
const Paper = React.forwardRef(function Paper(props, ref) {
const {
classes,
className: classNameProp,
Expand All @@ -53,8 +53,6 @@ const Paper = React.forwardRef((props, ref) => {
return <Component className={className} ref={ref} {...other} />;
});

Paper.displayName = 'Paper';

Paper.propTypes = {
/**
* The content of the component.
Expand Down