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

Tooltip triggers re-renders by repeatedly setting event handlers #21382

Closed
2 tasks done
rassie opened this issue Jun 9, 2020 · 8 comments
Closed
2 tasks done

Tooltip triggers re-renders by repeatedly setting event handlers #21382

rassie opened this issue Jun 9, 2020 · 8 comments
Labels
component: tooltip This is the name of the generic UI component, not the React module! discussion performance

Comments

@rassie
Copy link

rassie commented Jun 9, 2020

While analyzing why-did-you-render's output on my project, I've noticed quite a lot of re-renders due to changed event handlers in the props of one of my components, like props.onTouchEnd, props.onFocus, props.onMouseOver etc., which are according to WDYR different functions with same name. Since I do no such thing in my component, I've investigated and found out that those properties are set by Tooltip, which is wrapped around my component .

It is possible that this is a false positive by why-did-you-render, which I'll happily report as a bug in that project. However, I'm unable to either prove or disprove this and would appreciate a bit of help.

I suspect this problem has something to do with my usage of useCallback for onPointerEnter and onPointerLeave, however, my understanding is that this usage is legal and even preferred. I'd appreciate any pointers saying otherwise.

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

If WDYR is to be believed, Tooltip triggers quite a lot of re-renders by setting pointer events on the wrapped components repeatedly.

Expected Behavior 🤔

No such re-renders.

Steps to Reproduce 🕹

Steps:

  1. Open https://codesandbox.io/s/priceless-cache-den79
  2. Look into console output.

Your Environment 🌎

Tech Version
Material-UI v4.9.10
React 16.13.1
Browser
TypeScript 3.8.3
etc.
@rassie rassie added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jun 9, 2020
@joshwooding
Copy link
Member

Had a quick look - only seems to log when an icon is the direct child of the tooltip, seems okay with an IconButton. On React Devtools seems like everything is fine things get re-rendered when opening/closing the tooltip due to the title and aria-describedby prop changing.

I wouldn't worry around counting renders anyway until you start seeing performance issues.

Overall looks like everything is working fine to me.

@rassie
Copy link
Author

rassie commented Jun 10, 2020

So basically, if I'm willing to avoid the noise, I could wrap the Icon in a Fragment and be done with it?

@eps1lon eps1lon added discussion performance and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jun 10, 2020
@eps1lon
Copy link
Member

eps1lon commented Jun 10, 2020

We need to inject some handlers into the tooltip child. We don't memoize them since we assume that the children are fairly cheap to render. In the end you don't wrap tooltips around big UI elements.

Do you have an example of an expensive child of a Tooltip that would warrant aggressively memoizing?

@rassie
Copy link
Author

rassie commented Jun 10, 2020

I don't really know it that counts as expensive, but I could imagine a dynamically generated and fetched thumbnail which needs to show its properties on mouse hover (think some kind of photo editor).

@rassie
Copy link
Author

rassie commented Jun 10, 2020

So basically, if I'm willing to avoid the noise, I could wrap the Icon in a Fragment and be done with it?

Just checked, a Fragment does not work, the tooltip doesn't appear. However, <div> helps.

@eps1lon
Copy link
Member

eps1lon commented Jun 10, 2020

I could imagine a dynamically generated and fetched thumbnail which needs to show its properties on mouse hover (think some kind of photo editor).

I don't follow how a re-render makes this expensive.

Anyway, we can't act on theoretical issues. We could spend months thinking about edge cases that could make this a performance bottleneck and I'm fairly certain you could fix performance issues without imposing memoization for 99.9% of the use cases.

If you have a concrete use case, please share it here. Until then I'm closing.

@eps1lon eps1lon closed this as completed Jun 10, 2020
@rassie
Copy link
Author

rassie commented Jun 10, 2020

No, thank you for the help and double-checking. I've got a solution I needed and it doesn't appear to be a bug, so we are done here.

@oliviertassinari oliviertassinari added the component: tooltip This is the name of the generic UI component, not the React module! label Aug 22, 2020
@zeorin
Copy link

zeorin commented Oct 17, 2023

I'd just like to mention that this is rather annoying if one memoizes everything and uses why-did-you-render to track unnecessary re-renders (trackAllPureComponents: true).

It is possible to work around this by filtering out these specific props being set by Tooltip specifically, but it is very cumbersome to do so (partly that's also because why-did-you-render currently has a bug in their API that doesn't expose additionalOwnerData even when providing a getAdditionalOwnerData function).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: tooltip This is the name of the generic UI component, not the React module! discussion performance
Projects
None yet
Development

No branches or pull requests

5 participants