[Portal] Prepare deprecation of onRendered #16597

oliviertassinari · 2019-07-14T09:38:33Z

Starting now, people can use the ref instead of the onRendered prop. I think that we should remove it in v5.

mui-pr-bot · 2019-07-14T09:48:16Z

Details of bundle changes.

Comparing: b6182ce...0a78aca

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	+0.02% 🔺	+0.01% 🔺	327,251	327,315	90,345	90,355
@material-ui/core/Paper	0.00%	0.00%	68,477	68,477	20,410	20,410
@material-ui/core/Paper.esm	0.00%	0.00%	61,761	61,761	19,177	19,177
@material-ui/core/Popper	+0.20% 🔺	+0.20% 🔺	28,896	28,955	10,394	10,415
@material-ui/core/Textarea	0.00%	-0.08%	5,534	5,534	2,369	2,367
@material-ui/core/TrapFocus	0.00%	0.00%	3,753	3,753	1,577	1,577
@material-ui/core/styles/createMuiTheme	0.00%	-0.03%	16,156	16,156	5,816	5,814
@material-ui/core/useMediaQuery	0.00%	0.00%	3,098	3,098	1,310	1,310
@material-ui/lab	+0.04% 🔺	+0.03% 🔺	141,699	141,758	43,813	43,824
@material-ui/styles	0.00%	0.00%	51,886	51,886	15,380	15,380
@material-ui/system	0.00%	-0.25%	15,576	15,576	4,445	4,434
Button	0.00%	-0.03%	79,711	79,711	24,358	24,350
Modal	+0.12% 🔺	-0.04%	14,548	14,566	5,102	5,100
Portal	+0.37% 🔺	-0.26%	3,471	3,484	1,568	1,564
Rating	0.00%	-0.03%	70,267	70,267	22,068	22,062
Slider	0.00%	-0.02%	75,096	75,096	23,311	23,306
colorManipulator	0.00%	0.00%	3,904	3,904	1,543	1,543
docs.landing	0.00%	0.00%	54,357	54,357	13,915	13,915
docs.main	+0.01% 🔺	+0.01% 🔺	646,774	646,834	203,034	203,057
packages/material-ui/build/umd/material-ui.production.min.js	+0.01% 🔺	0.00%	299,666	299,706	86,112	86,114

Generated by 🚫 dangerJS against 0a78aca

packages/material-ui/src/Portal/Portal.js

packages/material-ui/src/Modal/Modal.js

packages/material-ui/src/Portal/Portal.js

Updated to take the reviews into account

eps1lon · 2019-07-16T13:17:10Z

packages/material-ui/src/Portal/Portal.js


  useEnhancedEffect(() => {
    if (!disablePortal) {
      setMountNode(getContainer(container) || document.body);
    }
  }, [container, disablePortal]);

-  React.useImperativeHandle(ref, () => mountNode || childRef.current, [mountNode]);
+  useEnhancedEffect(() => {


Why can't we use useImperativeHandle here?

Yes, probably with the new if(!node) logic. Will try +1

Oh no, we can't. my bad. We still need to ignore the case where disablePortal is false.

Could you elaborate on that? Right now we handle a ref in the commit phase. If it's not possible to handle in the ref phase we need to name it differently. Otherwise it might create confusing bugs because it's expected to be updated during ref phase when it only happens during commit.

Yes, with pleasure. The problem is in the case where disablePortal is true. The classic ref logic will try to set the value to the DOM node during the first ref phase. However, the useImperativeHandle logic also tries to set the value to null. It creates a conflict. I believe it can't work.

Also, I'm wondering does it care in which phase the ref value is set in our case? Previously, the ref was set to null during the ref phase (to accommodate SSR). Now, the ref isn't called. In both cases, people can't rely on the ref value to be defined, they have to handle this undefined case. They have to wait for the actual value, after the first commit, ref, layout and effect phases.

packages/material-ui/src/Portal/Portal.test.js

eps1lon · 2019-07-16T13:22:59Z

packages/material-ui/src/Portal/Portal.js

@@ -65,6 +73,8 @@ Portal.propTypes = {
  disablePortal: PropTypes.bool,
  /**
   * Callback fired once the children has been mounted into the `container`.
+   *
+   * This prop will be deprecated and removed in v5, the ref can be used instead.


Suggested change

* This prop will be deprecated and removed in v5, the ref can be used instead.

* You probably don't need this prop. Prefer to use callback refs on the component instead.

We haven't deprecated it yet.

The motivation for the "will be deprecate" is to create a sentiment of urgency for somebody relying on the prop but has a use case (we are not aware of) that can't rely on the ref. Do you think that we should still go for the "probably not"? I'm taking the time to ask because it's a broader topic, not specific to this occurrence. I can remember that the same method is used for withWidth. We had people opening an issue "Please don't deprecate withWidth" with some details (explaining why) consequent to the initial ultimatum.

I could only find usage of it in Popper and that component has quite the complicated implementation for pretty simple behavior. I tried reducing the amount of logic to get a better hold of why we need onRendered instead of ref.

One part of the issue is the misnomer. Makes it hard to mentally "click" what this prop does.

I'd like to explore first why we need it in Popper and then we can go back to this one. As it looks like right now "you probably don't need it" is the better advise.

The motivation for the "will be deprecate" is to create a sentiment of urgency for somebody relying on the prop

That is not the why but the what. Why do you want to create a sense of urgency?

I had forgotten about the Popper. I believe that the child div ref could be used instead. Would it work?

Why do you want to create a sense of urgency?

It incentives users to give us feedback sooner than if we wait to introduce the deprecation. it optimizes for learning. The sooner we have the information:

the less likely we will introduce a deprecation that we have to revert

the less likely somebody will use the prop and the better migration path going from v4 to v5

Not sure what we gain by saying we will deprecate it if we're not sure about it. That can only hurt trust.

it optimizes for learning.

Could you elaborate on that?

Sorry, I wasn't clear.

I think that we should first agree that removing the prop is the best long-term path. Do we?

Then, it's about waiting x months (x because we don't want people to feel too much code change pressure) before introducing the deprecation warning vs saying it right away and adding a warning only later on (it creates x months of additional learning opportunity).

I don't think that saying the prop will be deprecated and not deprecating it is worse than deprecating a prop and reverting.

So, if you think that we are not ready yet to commit to a deprecation, your suggestion is better without any doubts. If you think that we are good for deprecation, we have an opportunity to collect feedback x more months ("optimize for learning").

It should be good to go now.

oliviertassinari · 2019-07-19T10:57:06Z

I have removed the onRendered prop usage from the Popper.js component. It seems to work OK.

oliviertassinari · 2019-07-19T19:07:13Z

Rebased.

oliviertassinari added core component: Portal The React component. labels Jul 14, 2019

oliviertassinari mentioned this pull request Jul 14, 2019

Portal: onRendered doesn't make sense if disablePortal is true #16595

Closed

2 tasks

oliviertassinari force-pushed the modal-portal-ref branch from b6d23ae to 5c072f1 Compare July 14, 2019 09:41

oliviertassinari requested a review from eps1lon July 14, 2019 09:46

eps1lon previously requested changes Jul 14, 2019

View reviewed changes

packages/material-ui/src/Portal/Portal.js Outdated Show resolved Hide resolved

packages/material-ui/src/Modal/Modal.js Show resolved Hide resolved

Zaynex reviewed Jul 14, 2019

View reviewed changes

packages/material-ui/src/Portal/Portal.js Outdated Show resolved Hide resolved

oliviertassinari force-pushed the modal-portal-ref branch 3 times, most recently from 445b94b to a9c5612 Compare July 16, 2019 08:42

eps1lon previously requested changes Jul 16, 2019

View reviewed changes

oliviertassinari force-pushed the modal-portal-ref branch from a9c5612 to de60732 Compare July 17, 2019 10:02

oliviertassinari mentioned this pull request Jul 18, 2019

Material-UI 1.2.1 not working with Preact preactjs/preact-compat#476

Closed

oliviertassinari force-pushed the modal-portal-ref branch from de60732 to 5042c69 Compare July 19, 2019 10:34

oliviertassinari force-pushed the modal-portal-ref branch from 5042c69 to 76c656f Compare July 19, 2019 19:07

oliviertassinari added 4 commits July 19, 2019 23:25

[Portal] Prepare deprecation of onRendered

2e7f18e

review

ac7f33a

breakdown the tests

dc82e61

remove onRendered usage in the Popper component

0a78aca

oliviertassinari force-pushed the modal-portal-ref branch from 76c656f to 0a78aca Compare July 19, 2019 21:25

oliviertassinari added the PR: good for merge label Jul 20, 2019

oliviertassinari merged commit faad8dd into mui:master Jul 22, 2019

oliviertassinari deleted the modal-portal-ref branch July 22, 2019 21:54

snyk-bot mentioned this pull request Sep 4, 2019

[Snyk] Upgrade @material-ui/styles from 4.3.0 to 4.3.3 iliutastoica/React-Discord-Clone#5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Portal] Prepare deprecation of onRendered #16597

[Portal] Prepare deprecation of onRendered #16597

oliviertassinari commented Jul 14, 2019 •

edited

Loading

mui-pr-bot commented Jul 14, 2019 •

edited

Loading

eps1lon Jul 16, 2019

oliviertassinari Jul 16, 2019

oliviertassinari Jul 17, 2019

eps1lon Jul 17, 2019

oliviertassinari Jul 17, 2019

eps1lon Jul 16, 2019

oliviertassinari Jul 16, 2019 •

edited

Loading

eps1lon Jul 17, 2019

oliviertassinari Jul 17, 2019

eps1lon Jul 17, 2019

oliviertassinari Jul 17, 2019 •

edited

Loading

oliviertassinari commented Jul 19, 2019

oliviertassinari commented Jul 19, 2019

	* This prop will be deprecated and removed in v5, the ref can be used instead.
	* You probably don't need this prop. Prefer to use callback refs on the component instead.

[Portal] Prepare deprecation of onRendered #16597

[Portal] Prepare deprecation of onRendered #16597

Conversation

oliviertassinari commented Jul 14, 2019 • edited Loading

mui-pr-bot commented Jul 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Jul 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Jul 17, 2019 • edited Loading

Choose a reason for hiding this comment

oliviertassinari commented Jul 19, 2019

oliviertassinari commented Jul 19, 2019

oliviertassinari commented Jul 14, 2019 •

edited

Loading

mui-pr-bot commented Jul 14, 2019 •

edited

Loading

oliviertassinari Jul 16, 2019 •

edited

Loading

oliviertassinari Jul 17, 2019 •

edited

Loading