[styles] Improve ref forwarding by eps1lon · Pull Request #13676 · mui/material-ui

eps1lon · 2018-11-23T09:45:09Z

Breaking change

refs that are attached to components decorated with withStyles and withTheme now forward their refs to the decorated component:

const Component = React.forwardRef((props, ref) => <div {...props} ref={ref} />);
const StyledComponent = withStyles({})(Component);
const ref = React.createRef();
- const rendered = <StyledComponent innerRef={ref} />;
+ const rendered = <StyledComponent ref={ref} />;

If you wrapped a function component and used ref on the wrapped component to get a DOM node via ref use RootRef instead if you have no control over the component

const Component = props => <div {...props} />
const StyledComponent = withStyles({})(Component);
- const rendered = <StyledComponent ref={ref} />;
+ const rendered = <RootRef ref={ref}><StyledComponent ref={ref} /></RootRef>;

or React.forwardRef if you do have control:

- const Component = props => <div {...props} />
+ const Component = React.forwardRef((props, ref) => <div ref={ref} {..props} />);
const StyledComponent = withStyles({})(Component);
const rendered = <StyledComponent ref={ref} />;

innerRef is being deprecated. It is however prioritized over ref:

const StyledComponent = withStyles({})(Component);
const ref = React.createRef();
const innerRef = React.createRef();
const rendered = <StyledComponent ref={ref} innerRef={innerRef} />;
console.assert(ref.current == null);
console.assert(innerRef.current != null);

@material-ui/styles is still in alpha so the breaking change should not require a major version bump. It will be however breaking since we will use @material-ui/styles in v4 over @material-ui/core/styles.

eps1lon · 2018-11-23T18:11:58Z

hoist-non-react-static < 3.0.0 is holding this back. My guess is that react-router is the main offender but we might have to fix every dependency that is using the old hoist version that doesn't support React.forwardRef.
Fixed with d3b113a

oliviertassinari

After this change, I could notice that the ripple is no longer working, that opening a menu crash the page, that we can some new warnings in the console:

eps1lon · 2018-11-24T18:15:15Z

@oliviertassinari
We really do need to approach this bottom-up.

Currently things like <Typography innerRef={ref} /> are not working because the naked component is a function component. So enabling this pattern (which is currently permitted in our ts declarations) should be our first step. Need to put this here back on hold.

oliviertassinari · 2018-11-27T19:21:27Z

@eps1lon How do you want to move forward?

eps1lon · 2018-11-27T19:36:26Z

collect every component that we attach refs to
transform those into forwardRef so that we could use e.g. <List innerRef={ref} /> (currently working on)
make sure withStyles forwards refs (with option to disable this behavior). At this point we can use <List ref={ref} /> [styles] Improve ref forwarding #13676
Change findDOMNode calls so that they are done on a ref to a DOM node rather than this or a component instance (for ButtonBase this is done in [ButtonBase] Require host or ref forwarding components #13664 but we need to solve 1 and 2 first.)
That's hardest part. I looked into some components and it's sometimes pretty scary what they are doing (e.g. Slide). In other instances we probably have to to switch from React.Fragment and findDOMNode to additional markup to which we can attach a ref.

oliviertassinari · 2018-11-27T19:43:35Z

Change findDOMNode calls so that they are done on a ref to a DOM node

Sounds good, it's large enterprise, thank you for leading it! Do we still need findDOMNode once we have a reference to the dom node?

eps1lon · 2018-11-27T19:49:33Z

I think we can keep it if users do something like <Button component={SomeClassComponent} />. It's better backwards compatibility for no real cost (beyond 2 strict equality checks) if users take care about ref forwarding.

eps1lon · 2019-02-01T16:04:32Z

Alright the issues described by @oliviertassinari are now solved. Only caveat is a deprecation warning in our docs now for ButtonBase since that component uses innerRef which is deprecated in @material-ui/styles with this PR. It's working perfectly fine and until we switch to @material-ui/styles in @material-ui/core we need this bridge.

eps1lon · 2019-02-05T16:37:38Z

- It forwards *non React static* properties so this HOC is more "transparent".
+- It forwards refs to the inner component.
+- The `innerRef` prop is deprecated. Use `ref` instead.
+- It does **not** copy over statics.


Forgotten in #13698

oliviertassinari · 2019-02-05T21:12:52Z

+        const ThemedDiv = withTheme()('div');
+
+        mount(
+          <>


There was an issue with refs on root components when mounting in enzyme. I think I added a comment somewhere but this might've been on a local branch.

@oliviertassinari I can't find the original issue unfortunately. However it does indeed break if I remove the fragment. ref.current will be undefined.

mui-pr-bot · 2019-03-05T12:47:45Z

@material-ui/styles: parsed: -3.06% 😍, gzip: -2.18% 😍

Details of bundle changes.

Comparing: 8d46415...ff974b3

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	-0.00%	+0.01% 🔺	371,833	371,832	91,926	91,939
@material-ui/core/Paper	-0.00%	+0.01% 🔺	76,649	76,648	19,307	19,308
@material-ui/core/Paper.esm	0.00%	-0.01%	71,599	71,599	18,784	18,783
@material-ui/core/Popper	0.00%	-0.03%	30,462	30,462	10,585	10,582
@material-ui/core/styles/createMuiTheme	0.00%	0.00%	17,286	17,286	5,717	5,717
@material-ui/core/useMediaQuery	0.00%	+0.19% 🔺	2,486	2,486	1,047	1,049
@material-ui/lab	-0.00%	+0.03% 🔺	184,282	184,281	50,570	50,584
@material-ui/styles	-3.06%	-2.18%	57,443	55,687	16,177	15,825
@material-ui/system	0.00%	-0.02%	17,062	17,062	4,483	4,482
Button	0.00%	-0.02%	99,409	99,409	26,612	26,607
Modal	0.00%	+0.01% 🔺	98,984	98,984	26,260	26,263
colorManipulator	0.00%	0.00%	3,232	3,232	1,296	1,296
docs.landing	+0.14% 🔺	+0.20% 🔺	51,773	51,845	11,244	11,266
docs.main	-0.08%	-0.03%	678,857	678,318	206,238	206,183
packages/material-ui/build/umd/material-ui.production.min.js	0.00%	0.00%	322,466	322,466	85,044	85,044

Generated by 🚫 dangerJS against ff974b3

oliviertassinari · 2019-03-05T17:40:55Z

@@ -1,4 +1,3 @@
-/* istanbul ignore file */


I'm removing all the istanbul ignore, @eps1lon is right, let's leave the 100% utopia.

To be fair that broken window analogy you linked made total sense to me. I just think that these comments won't help in that regard. At least this way we have the actual number to look at. Previously it was 100% (kind of; who knows how good it is actually). Now we know how good/bad we are and have tools helping us analyze it. Codesearch is not really that helpful IMO. It's like prefering // TODO over creating issues.

oliviertassinari · 2019-03-05T17:42:02Z

-        'Material-UI: The `innerRef` prop is deprecated and will be removed in v5. ' +
-          'Refs are now automatically forwarded to the inner component.',
-      );
+      // return new Error(


It's raising a warning as the ButtonBase is still using this API.

Right. I think I annotated it with @deprecated somewhere to indicate a "soft deprecation". In a perfect world the eslint rule would detect these. I don't like runtime deprecation warnings that have the same level as any other warning (hello ReactDOM.findDOMNode). A lint rule makes more sense to me.

oliviertassinari · 2019-03-05T17:43:12Z

      const ref = React.createRef();
      mount(
-        <>
+        <React.Fragment>


What should we do with the Fragment. Should we embrace <> everywhere, should only use React.Fragment or it doesn't matter 🤔. I'm reducing entropy until we decide.

I think React.Fragment is required if fragment refs land. So maybe enforce that?

oliviertassinari · 2019-03-05T17:54:02Z

I want to merge this pull request so I can finally complete #14560!

eps1lon added type: new feature Expand the scope of the product to solve a new problem. breaking change Introduces changes that are not backward compatible. scope: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. labels Nov 23, 2018

eps1lon requested a review from oliviertassinari November 23, 2018 09:45

eps1lon added the on hold There is a blocker, we need to wait. label Nov 23, 2018

eps1lon commented Nov 24, 2018

View reviewed changes

Comment thread babel.config.js Outdated

eps1lon removed the on hold There is a blocker, we need to wait. label Nov 24, 2018

oliviertassinari reviewed Nov 24, 2018

View reviewed changes

Comment thread packages/material-ui-styles/src/withTheme.test.js Outdated

Comment thread packages/material-ui-styles/src/withStyles.js Outdated

eps1lon added the on hold There is a blocker, we need to wait. label Nov 24, 2018

eps1lon mentioned this pull request Nov 28, 2018

[core] Enable innerRef on Backdrop, List, MenuList and Paper #13722

Merged

eps1lon changed the base branch from master to next February 1, 2019 15:24

eps1lon removed the on hold There is a blocker, we need to wait. label Feb 1, 2019

eps1lon mentioned this pull request Feb 5, 2019

[RFC] Refs in v4 #14415

Closed

99 tasks

eps1lon commented Feb 5, 2019

View reviewed changes

eps1lon mentioned this pull request Feb 5, 2019

[core] Enable innerRef on ListItem and MenuItem #14423

Merged

oliviertassinari reviewed Feb 5, 2019

View reviewed changes

eps1lon mentioned this pull request Feb 5, 2019

RFC: Material-UI v4 breaking changes #13663

Closed

56 tasks

[styles] Forward refs in with* hocs

4998d69

eps1lon force-pushed the fix/styles/withStyles/ref-forwarding branch from 3ea3061 to 4998d69 Compare March 5, 2019 13:11

[styles] Comment the deprecation, we are still using innerRef

435f396

oliviertassinari reviewed Mar 5, 2019

View reviewed changes

fix eslint warnings

ff974b3

oliviertassinari merged commit 3b6a66d into mui:next Mar 5, 2019

oliviertassinari mentioned this pull request Mar 8, 2019

[test] Increase the Codecov threshold #14796

Merged

eps1lon mentioned this pull request Mar 10, 2019

[docs] Remove unnecessary findDOMNode usage #14836

Merged

eps1lon deleted the fix/styles/withStyles/ref-forwarding branch March 18, 2019 07:53

Uh oh!

Uh oh!

Conversation

eps1lon commented Nov 23, 2018 • edited by oliviertassinari Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking change

Uh oh!

eps1lon commented Nov 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

oliviertassinari left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eps1lon commented Nov 24, 2018

Uh oh!

oliviertassinari commented Nov 27, 2018

Uh oh!

eps1lon commented Nov 27, 2018

Uh oh!

oliviertassinari commented Nov 27, 2018

Uh oh!

eps1lon commented Nov 27, 2018

Uh oh!

eps1lon commented Feb 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mui-pr-bot commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliviertassinari Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliviertassinari Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliviertassinari commented Mar 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

eps1lon commented Nov 23, 2018 •

edited by oliviertassinari

Loading

eps1lon commented Nov 23, 2018 •

edited

Loading

mui-pr-bot commented Mar 5, 2019 •

edited

Loading

oliviertassinari Mar 5, 2019 •

edited

Loading

oliviertassinari Mar 5, 2019 •

edited

Loading