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

Dialog, lazy and Suspense #15283

Closed
eps1lon opened this issue Apr 9, 2019 · 5 comments · Fixed by #15291
Closed

Dialog, lazy and Suspense #15283

eps1lon opened this issue Apr 9, 2019 · 5 comments · Fixed by #15291
Labels
bug 🐛 Something doesn't work component: tooltip This is the name of the generic UI component, not the React module! external dependency Blocked by external dependency, we can’t do anything about it

Comments

@eps1lon
Copy link
Member

eps1lon commented Apr 9, 2019

@eps1lon @oliviertassinari Is this issue fixed?

If not I just built this Sandbox.
The error has to be triggered in a certain way and it might not trigger every time.

  1. Hover over the Material-UI title until the tooltip appears.
  2. Quickly click on the button to load the dialog.

It seems to be a race condition with (timers?) i guess.
You need to inspect the iframe to get the real exception, cause i wasn't able to quickly fix the cross origin error.

Hope that helps.

https://codesandbox.io/s/54p08z2llx

Originally posted by @Obbi89 in #15136 (comment)

@eps1lon eps1lon added bug 🐛 Something doesn't work component: tooltip This is the name of the generic UI component, not the React module! labels Apr 9, 2019
@eps1lon
Copy link
Member Author

eps1lon commented Apr 9, 2019

Thank you for the report. I'm able to reproduce this with your codesandbox.

It might be an issue with react since findDOMNode throws inside componentDidUpdate with "Unable to find DOM node at unmounted component" which is definitely the wrong message.

I guess this is why findDOMNode is already deprecated. Will look into a reproducible example without material-ui and if this has already been reported.

@eps1lon
Copy link
Member Author

eps1lon commented Apr 9, 2019

Update: facebook/react#14188 (comment)

The "quickly click" part is likely causing an even number of render calls which causes the error.

You can still open it if you happen to get an even amount of renders 😉

@Obbi89
Copy link

Obbi89 commented Apr 9, 2019

Alright nice. Guess that's an React issue then and might get fixed in the future™
Are you planing on removing findDOMNode from MUI, react strict mode keeps whining.
I will disable lazy load for that component until it is fixed in React. Thanks.

@eps1lon
Copy link
Member Author

eps1lon commented Apr 9, 2019

Are you planing on removing findDOMNode from MUI, react strict mode keeps whining.

We're working on it. Removing the usage for the Tooltip is a bit rough though. It would require us to drop support of function components as children.

Though given the current bugs in react and bugs findDOMNode can cause in general it's probably for the better.

@eps1lon eps1lon added the external dependency Blocked by external dependency, we can’t do anything about it label Apr 10, 2019
@eps1lon
Copy link
Member Author

eps1lon commented Apr 10, 2019

This will be fixed in the next release of React. Specifically any release including facebook/react#15312

We're also removing the findDOMNode usage in the Tooltip so @material-ui/core@^4.0.0 (future stable release) will also include the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: tooltip This is the name of the generic UI component, not the React module! external dependency Blocked by external dependency, we can’t do anything about it
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants