[ButtonBase] Require host or ref forwarding components by eps1lon · Pull Request #13664 · mui/material-ui

eps1lon · 2018-11-21T15:30:33Z

BREAKING: components passed via component prop need to be able to hold refs.

oliviertassinari · 2018-11-21T19:18:59Z

@eps1lon I have no objection to this change. It makes me sad for the userland complexity it's adding but it's just following by the direction the React team took 🙏.

Most people are using react-router, what's the interoperability story with them? Should we update the ComponentProperty demo? remix-run/react-router#6195

I would vote for waiting for the last moment to merge this pull request. I have a secret hope that React will provide a better answer to the problem before we have to release this change.

eps1lon · 2018-11-21T19:48:23Z

Only really looked at this from an implementation point of view. Will look through all the usage again and investigate what plans react-router has for forwarding refs. Since NavLink is a functional component using forwardRef could be considered non-breaking and an actual feature which would help for users to move forward (no pun intended).

Since this is targeted at next anyway there is no point in holding of with merging just to wait for react. If they revert we can revert too.

oliviertassinari · 2018-11-21T19:52:15Z

Since this is targeted at next anyway there is no point in holding of with merging just to wait for react. If they revert we can revert too.

Good point, it doesn't target master.

eps1lon · 2018-11-22T16:15:14Z

@oliviertassinari

mui-pr-bot · 2019-03-07T15:37:06Z

Details of bundle changes.

Comparing: a5b8011...0e354a5

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	+0.05% 🔺	+0.03% 🔺	362,777	362,964	92,213	92,238
@material-ui/core/Paper	0.00%	+0.02% 🔺	68,878	68,878	20,170	20,175
@material-ui/core/Paper.esm	0.00%	-0.01%	63,340	63,340	19,272	19,270
@material-ui/core/Popper	0.00%	-0.02%	30,460	30,460	10,573	10,571
@material-ui/core/styles/createMuiTheme	0.00%	0.00%	17,284	17,284	5,700	5,700
@material-ui/core/useMediaQuery	0.00%	0.00%	2,471	2,471	1,035	1,035
@material-ui/lab	+0.11% 🔺	+0.10% 🔺	170,747	170,934	50,065	50,114
@material-ui/styles	0.00%	+0.02% 🔺	57,220	57,220	16,273	16,276
@material-ui/system	0.00%	0.00%	17,041	17,041	4,493	4,493
Button	+0.72% 🔺	+0.56% 🔺	91,152	91,812	26,939	27,089
Modal	0.00%	-0.02%	90,261	90,261	26,724	26,718
colorManipulator	0.00%	0.00%	3,232	3,232	1,298	1,298
docs.landing	+0.13% 🔺	+0.19% 🔺	51,908	51,976	11,302	11,324
docs.main	+0.04% 🔺	+0.05% 🔺	647,808	648,093	201,557	201,650
packages/material-ui/build/umd/material-ui.production.min.js	+0.05% 🔺	+0.14% 🔺	311,179	311,325	85,197	85,314

Generated by 🚫 dangerJS against 0e354a5

oliviertassinari

Should we replace the usage of buttonRef in the source and deprecate the ref?

oliviertassinari · 2019-03-07T17:09:37Z

        onTouchStart={this.handleTouchStart}
-        onContextMenu={this.handleContextMenu}
-        ref={innerRef || buttonRef}
+        ref={ref => {


What about creating an instance method not to call setRef every time the ButtonBase renders?

Got the same idea when I read the review below 😄 Will investigate if we can configure a lint rule to check for this.

This wouldn't handle change of props. This behavior is however required for the SpeedDial. Since it's an optimization we can tackle this in a different PR.

eps1lon · 2019-03-08T08:46:12Z

Should we replace the usage of buttonRef in the source and deprecate the ref?

I try to avoid as much breaking changes as possible. I think we can make this smoother by forwarding refs in 4.0 (soft deprecate innerRef¹ with code comment), hard deprecate innerRef in 4.1 (with runtime warning) and then remove them in 5.0.

¹This includes innerRef and similar ref props like inputRef or buttonRef.

oliviertassinari · 2019-03-08T11:54:31Z

@eps1lon The soft deprecation for v4 sounds good to me. It's still a moving field.

eps1lon · 2019-03-08T12:20:54Z

    // Fix for https://github.com/facebook/react/issues/7769
-    if (!this.button) {
-      this.button = event.currentTarget;
+    if (!this.buttonRef.current) {


This branch is not covered anymore. The behavior was never tested anyway though. Researching the correct behavior that we would have to mock should be very expensive considering the age of the react issue. It essentially removes a false positive line coverage report.

BREAKING: components passed via `component` prop need to forward their refs.

Did you know that Prettier supports the markdown format 😱.

This reverts commit c4d4378. Was premature optimization which can cause subtle bugs if props changes. SpeedDial inparticual was relying on this behavior. Should be tackled separately.

oliviertassinari · 2019-03-12T11:03:44Z

@@ -1,3 +1,4 @@
+/* eslint-disable react/no-multi-comp */


I'm not a fan of this eslint react rule. What do you think of disabling it globally? Creating small components can be useful, creating a new file is an overhead. Should each function be into a different file, probably not. We don't enfoce it for functions. Why enforcing it for React components?

Fully agree. It's not always clear when you should create an extra file. You could probably come up with some heuristic (LOC, number of instance fields or hooks etc) but what would that actually achieve?

I think this is one of those rules that you never explicitly follow until it rejects some code and then you disable it. Not useful at all IMO.

oliviertassinari · 2019-03-12T11:06:50Z

  /**
   * Use that property to pass a ref callback to the native button component.
-   * @deprecated
+   * @deprecated Use `ref` instead


So, we gonna use the deprecatedPropType() helper in v4.1?

That's the plan. The existence of this prop doesn't actually block us so we might as well defer a breaking change here so that migration is easier. I still have the typography v2 in mind and while the warnings were just to much in that case it is still considered something you have to address when upgrading.

oliviertassinari · 2019-03-12T11:08:43Z

  }

+  getButtonNode() {
+    return ReactDOM.findDOMNode(this.buttonRef.current);


Smart. I guess findDOMNode only warns if the variable isn't a DOM element.

I need to verify if that is actually tested in react or if there's confirmation for that behavior. It could very well be accidental at which point the whole findDOMNode migration becomes a lot trickier.

Can we ask somebody from the React team?

I'm writing a test and will submit this as a PR to react. We'll see what they'll say about it.

eps1lon added breaking change Introduces changes that are not backward compatible. component: ButtonBase The React component. labels Nov 21, 2018

eps1lon added this to the v4 milestone Nov 21, 2018

eps1lon force-pushed the fix/ButtonBase/findDOMNOde-deprecation branch from e2fad77 to 6ddae5b Compare November 22, 2018 07:42

oliviertassinari reviewed Nov 22, 2018

View reviewed changes

eps1lon mentioned this pull request Nov 27, 2018

[styles] Improve ref forwarding #13676

Merged

mbrookes added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Feb 5, 2019

eps1lon force-pushed the fix/ButtonBase/findDOMNOde-deprecation branch from 64c0c62 to e23133d Compare March 7, 2019 15:31

eps1lon force-pushed the fix/ButtonBase/findDOMNOde-deprecation branch from f40a192 to 52b11f5 Compare March 7, 2019 16:27

eps1lon removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Mar 7, 2019

eps1lon requested a review from oliviertassinari March 7, 2019 16:44

oliviertassinari reviewed Mar 7, 2019

View reviewed changes

eps1lon commented Mar 8, 2019

View reviewed changes

Comment thread docs/src/pages/guides/composition/composition.md

eps1lon force-pushed the fix/ButtonBase/findDOMNOde-deprecation branch from f15df47 to df06956 Compare March 8, 2019 09:24

oliviertassinari mentioned this pull request Mar 8, 2019

[test] Increase the Codecov threshold #14796

Merged

eps1lon commented Mar 8, 2019

View reviewed changes

eps1lon added 7 commits March 10, 2019 17:19

[ButtonBase] Ensure react 16.6 strict mode compat

18b0299

BREAKING: components passed via `component` prop need to forward their refs.

[docs] Fix findDOMNode deprecation wrt ButtonBase

87b9468

[ButtonBase] Document ref fowarding requirement

2102cde

[docs] Document ref forwarding with react-router-dom

29e86f2

[docs] Add section about ref forwarding

2de0f87

consistent forwardRef composition pattern

c23c6ed

use recommended pattern of creating the component outside render

a998c6e

eps1lon and others added 7 commits March 10, 2019 17:20

[docs] Add simple strict mode feature flag

e4b5f33

[docs] Fix function components cannot be given refs

c4c0537

human prettier

a3b2556

Did you know that Prettier supports the markdown format 😱.

[ButtonBase] Document buttonRef deprecation

06339ba

[ButtonBase] Fix ref callbacks being triggered on every render

e506a7d

Revert "[ButtonBase] Fix ref callbacks being triggered on every render"

e3d941a

This reverts commit c4d4378. Was premature optimization which can cause subtle bugs if props changes. SpeedDial inparticual was relying on this behavior. Should be tackled separately.

[ButtonBase] Remove unnecessary rename on destructuring

0e354a5

eps1lon force-pushed the fix/ButtonBase/findDOMNOde-deprecation branch from df06956 to 0e354a5 Compare March 10, 2019 16:20

eps1lon requested a review from oliviertassinari March 12, 2019 11:02

oliviertassinari approved these changes Mar 12, 2019

View reviewed changes

oliviertassinari added the PR: good for merge label Mar 12, 2019

oliviertassinari mentioned this pull request Mar 13, 2019

[MenuItem] Add buttonRef (and other button props) type #14772

Merged

1 task

eps1lon merged commit 73653a7 into mui:next Mar 13, 2019

eps1lon deleted the fix/ButtonBase/findDOMNOde-deprecation branch March 13, 2019 20:25

eps1lon changed the title ~~[ButtonBase] Fix findDOMNode deprecation warnings~~ [ButtonBase] Require host or ref forwarding components Mar 13, 2019

joshwooding mentioned this pull request May 22, 2019

[ButtonBase] Convert to function component #15716

Merged

4 tasks

eps1lon mentioned this pull request May 26, 2019

Button jest snapshot testing fails with error, Invariant Violation: Unable to find node on an unmounted component #14069

Closed

2 tasks

Uh oh!

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

eps1lon commented Nov 21, 2018

Uh oh!

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

Uh oh!

Uh oh!

eps1lon commented Nov 22, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mui-pr-bot commented Mar 7, 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!

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!

Uh oh!

eps1lon commented Mar 8, 2019

Uh oh!

oliviertassinari commented Mar 8, 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!

Choose a reason for hiding this comment

Uh oh!

oliviertassinari Mar 12, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eps1lon commented Nov 21, 2018 •

edited

Loading

oliviertassinari commented Nov 21, 2018 •

edited

Loading

oliviertassinari commented Nov 21, 2018 •

edited

Loading

mui-pr-bot commented Mar 7, 2019 •

edited

Loading

oliviertassinari Mar 12, 2019 •

edited

Loading