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

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Feb 6, 2019

Cherry picked logic from reacts shared/getComponentName to handle forwardRef components.

Allows to skip setting displayName manually which also affected production bundles. We can remove this logic while retaining a meaningful displayName in a dev environment.

I suspect that this also helps folks that used display name selectors in enzyme and problematic testing habits. They should now get original e.g. Backdrop function component instead of the forwardRef wrapper.

@eps1lon eps1lon added the package: utils Specific to the @mui/utils package label Feb 6, 2019
@eps1lon eps1lon added this to the v4 milestone Feb 6, 2019
@@ -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.

@eps1lon eps1lon force-pushed the feat/utils/getDisplayName/enable-forwardRef branch from e1f5ebd to 6a03bd6 Compare February 6, 2019 09:44
@@ -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.

@@ -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?

},
{
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.

@eps1lon eps1lon merged commit 9199e12 into mui:next Feb 6, 2019
@eps1lon eps1lon deleted the feat/utils/getDisplayName/enable-forwardRef branch February 6, 2019 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: utils Specific to the @mui/utils package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants