[core] forward innerRef for certain components by eps1lon · Pull Request #14536 · mui/material-ui

eps1lon · 2019-02-14T20:46:55Z

It's recommended to review one commit at a time.

Two reasons I already open this:

I wanted to see what Draft Pull Request offers
I consider the individual commits itself complete. If anybody has some spare time early review would be more than welcome.

Breaking

Wraps components in forwardRef. Since every component is wrapped in withStyles this will only affect the innerRef prop. They will no longer return a ref to the instance (or error for function components) but rather the underlying DOM node where useful¹.

Some components are not included (yet). Those are the ones where virtually no test was passing so I defer them to a separate PR:

The initial change broke about 25% of our unit tests. Some of them were due to own mistakes, some because forwardRef was not appropriate but overall I suspect 15% of the breakage was due to flawed tests. I think https://blog.kentcdodds.com/why-i-never-use-shallow-rendering-c08851a68bb7 is a pretty good starting point to explain why.

Implementation details

function components are simply wrapped in their original function. I may rename the component var names later because const Foo = React.forwardRef(function Foo() {}) is quite tricky WRT to what Foo actually references
class components are wrapped in a withForwardRef HOC that injects the ref from forwardRef into innerRef. Since that prop is intercepted by withStyles it does not cause any property collision.

Notes for reviewers

Looking through one function and one class component implementation is sufficient. The rest follow the same pattern. The critical parts are the test files.

¹ This excludes TouchRipple. We need imperative handles here (see useImperativeHandle to trigger pulsating).

oliviertassinari · 2019-02-14T22:04:38Z

Could we run the CI?

eps1lon · 2019-02-14T22:08:50Z

Could we run the CI?

Sure. But it's far from ready yet and I didn't want to waste resources.

oliviertassinari · 2019-02-14T22:16:09Z

Sure. But it's far from ready yet and I didn't want to waste resources.

@eps1lon Thanks, I was interested in the bundle size.

My biggest issue is how we gonna educate people to correctly apply the ref when implementing custom components. 90% of the libraries of the community doesn't correctly forward the reference. I have just tested react-router. I can't get a ref on the DOM an element when using the Link. They don't seem to be interested in solving the problem: remix-run/react-router#6195. Same push back from reactjs/react-transition-group#457. When I all the changes needed, maybe the best move it to "wait and see" 🤔 but it has almost been 1 year since: #10825 (comment).

eps1lon · 2019-02-14T22:36:54Z

You can patch react-router-dom by wrapping it in another component: https://github.com/mui-org/material-ui/pull/13664/files#diff-52b99e4a854fd9a4fc16ebc3353cd14dR131

This approach should work with most libraries. As far as I know most of the libraries have some sort of innerRef prop.

Come to think of it it might be possible to check if a component allows refs to the DOM node (forwardRef or string elementType) and use ref in those cases, otherwise assume innerRef works. Will need to check how much complexity this adds.

Overall even if this adds to much complexity we can still cherry-pick changes from here. Things like aria-disabled on non-html Button or a more robust test suite is probably a good thing.

mui-pr-bot · 2019-02-21T17:27:21Z

@material-ui/core: parsed: +0.81% , gzip: +0.44%
@material-ui/lab: parsed: +0.70% , gzip: +0.71%

Details of bundle changes.

Comparing: 56fcebb...827b466

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	+0.81% 🔺	+0.44% 🔺	369,244	372,227	91,572	91,973
@material-ui/core/Paper	+0.37% 🔺	+0.40% 🔺	76,648	76,931	19,296	19,373
@material-ui/core/Paper.esm	+0.01% 🔺	+0.03% 🔺	71,596	71,601	18,774	18,779
@material-ui/core/Popper	0.00%	+0.02% 🔺	30,462	30,462	10,580	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.10% 🔺	2,486	2,486	1,050	1,051
@material-ui/lab	+0.70% 🔺	+0.71% 🔺	183,226	184,506	50,374	50,730
@material-ui/styles	+0.04% 🔺	+0.02% 🔺	57,708	57,733	16,229	16,233
@material-ui/system	0.00%	0.00%	17,062	17,062	4,485	4,485
Button	+0.56% 🔺	+0.59% 🔺	99,076	99,633	26,477	26,634
Modal	+0.01% 🔺	+0.03% 🔺	98,865	98,871	26,218	26,227
colorManipulator	0.00%	-0.08%	3,232	3,232	1,297	1,296
docs.landing	+0.07% 🔺	+0.17% 🔺	49,899	49,934	10,728	10,746
docs.main	+0.19% 🔺	+0.17% 🔺	678,034	679,298	205,987	206,347
packages/material-ui/build/umd/material-ui.production.min.js	+0.75% 🔺	+0.46% 🔺	320,385	322,787	84,772	85,165

Generated by 🚫 dangerJS against 827b466

oliviertassinari

Great work! I'm rebasing on next so we can merge :)

- forwardRef for TouchRipple was a bad idea. We need the imperative handles of the class

Caused all tests to fail. Defering until absolutely necessary

[skip ci]

- added accidentally deleted component JSDOCs - ignored innerRef from withForwardRef HOC

oliviertassinari · 2019-03-01T10:41:01Z

@eps1lon We have lost some test coverage:

I'm not sure yet what's going on. Codecov reports 100% test coverage, and yet, it's not the case after the merge. Can we trust codecov 🤔?

Little bit different approach this time: Refactored tests first then implementation. Definitely still biased because I knew how the implementation would look like but still increased confidence in tests for me. I need to investigate the shallow + disableLifeCycleIssue. This is causing enzyme to call componentDidMount if we wrap it in `forwardRef`. Continues #14536

Continues #14536

eps1lon added core breaking change Introduces changes that are not backward compatible. labels Feb 14, 2019

eps1lon added this to the v4 milestone Feb 14, 2019

eps1lon force-pushed the feat/forwar-ref branch from 87273f5 to 49247e9 Compare February 18, 2019 11:08

oliviertassinari mentioned this pull request Feb 19, 2019

[Tooltip] create around RootRef #14574

Closed

2 tasks

eps1lon force-pushed the feat/forwar-ref branch 5 times, most recently from e426edc to 786fcf5 Compare February 21, 2019 17:22

eps1lon force-pushed the feat/forwar-ref branch 3 times, most recently from 730364e to 6491c4c Compare February 24, 2019 19:31

eps1lon marked this pull request as ready for review February 24, 2019 19:42

eps1lon requested a review from oliviertassinari February 26, 2019 12:17

eps1lon added 2 commits February 28, 2019 21:45

[core] Move hoistInternalStatics into core

0f5967f

[core] add withForwardedRef hoc

f987a01

oliviertassinari reviewed Feb 28, 2019

View reviewed changes

eps1lon added 7 commits February 28, 2019 22:18

[core] forward ref to root component

645c3cd

[Grid] Fix failing test

cc2ee61

[ButtonBase] Use aria-disabled if disabled is not valid

db37607

[ExpansionPanelSummary] Fix tests

8ee275f

[SwitchBase] Fix failing tests

2eaa732

- forwardRef for TouchRipple was a bad idea. We need the imperative handles of the class

[OutlinedInput] Fix failing test

1f6caac

[core] Revert forwardRef for modal and dialog

426bc7a

Caused all tests to fail. Defering until absolutely necessary

eps1lon added 12 commits February 28, 2019 22:21

[FilledInput] Fix tests

ca047b6

[GridListTile] Fix failing tests

f33ed34

[Input] Fix test

4c54e65

[Textarea] Revert forwardRef

e2c862e

[InputLabel] Fix tests

94d3630

[Menu] Fix menu tests

1b3039c

[skip ci]

[Snackbar] Fix tests [skip ci]

e7f4372

[TablePagination] Fix tests broken due to rebase

986ec2c

[core] Format rebased files

08115ed

[docs] Fix API docs

a3331fe

- added accidentally deleted component JSDOCs - ignored innerRef from withForwardRef HOC

[core] Fix lint errors

05bf7d8

[lab] Use forwardRef

eadc6b9

oliviertassinari force-pushed the feat/forwar-ref branch from 6491c4c to eadc6b9 Compare February 28, 2019 21:31

oliviertassinari added the PR: good for merge label Feb 28, 2019

oliviertassinari force-pushed the feat/forwar-ref branch from 3e4c4d1 to f8e6881 Compare February 28, 2019 21:35

fix rebase issue

827b466

oliviertassinari force-pushed the feat/forwar-ref branch from f8e6881 to 827b466 Compare February 28, 2019 21:37

oliviertassinari merged commit 8a13bc2 into mui:next Mar 1, 2019

oliviertassinari mentioned this pull request Mar 1, 2019

[Table] Add dense support #14561

Merged

1 task

This was referenced Mar 1, 2019

Improve test coverage #14708

Closed

[Tabs] Forward refs #14714

Merged

eps1lon mentioned this pull request Mar 4, 2019

[Popover] Forward refs #14737

Merged

eps1lon deleted the feat/forwar-ref branch March 4, 2019 16:35

eps1lon mentioned this pull request Mar 4, 2019

[Modal] Forward refs #14738

Merged

eps1lon added a commit that referenced this pull request Mar 5, 2019

[Modal] Forward refs (#14738)

d4425c6

Continues #14536

This was referenced Mar 6, 2019

[Dialog] Forward refs #14775

Merged

[RFC] Refs in v4 #14415

Closed

[docs] Remove unnecessary findDOMNode usage #14836

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[core] forward innerRef for certain components#14536

[core] forward innerRef for certain components#14536
oliviertassinari merged 35 commits into
mui:nextfrom
eps1lon:feat/forwar-ref

eps1lon commented Feb 14, 2019 •

edited

Loading

Uh oh!

oliviertassinari commented Feb 14, 2019

Uh oh!

eps1lon commented Feb 14, 2019

Uh oh!

oliviertassinari commented Feb 14, 2019 •

edited

Loading

Uh oh!

eps1lon commented Feb 14, 2019

Uh oh!

mui-pr-bot commented Feb 21, 2019 •

edited

Loading

Uh oh!

oliviertassinari left a comment

Uh oh!

oliviertassinari commented Mar 1, 2019 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Conversation

eps1lon commented Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking

Implementation details

Notes for reviewers

Uh oh!

oliviertassinari commented Feb 14, 2019

Uh oh!

eps1lon commented Feb 14, 2019

Uh oh!

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

Uh oh!

Uh oh!

eps1lon commented Feb 14, 2019

Uh oh!

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

Uh oh!

Uh oh!

oliviertassinari left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

eps1lon commented Feb 14, 2019 •

edited

Loading

oliviertassinari commented Feb 14, 2019 •

edited

Loading

mui-pr-bot commented Feb 21, 2019 •

edited

Loading

oliviertassinari commented Mar 1, 2019 •

edited

Loading