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

[core] Only use debounce client-side #13255

Merged
merged 1 commit into from
Oct 15, 2018

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Oct 15, 2018

Win ~5% of server-side rendering performance per debounce deferred to the client. For instance, around 10% for the tab that uses two debounce.

@oliviertassinari oliviertassinari force-pushed the debounce-client-side branch 5 times, most recently from f919d3d to 6c27ae6 Compare October 15, 2018 20:16
@oliviertassinari oliviertassinari merged commit 402683a into mui:master Oct 15, 2018
@oliviertassinari oliviertassinari deleted the debounce-client-side branch October 15, 2018 20:27
defaultTheme = createMuiTheme();
defaultTheme = createMuiTheme({
typography: {
suppressWarning: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oliviertassinari
I just found this. Why was this snuck in in this PR? We had this discussion over and over. If you don't care about deprecation warnings just say it and we can leave this discussion behind. I'm tired of watching every commit so that you don't work against the determined deprecation behavior in the RFC.

no more deprecation warnings

Copy link
Member Author

@oliviertassinari oliviertassinari Nov 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The motivation is the same of the fix proposed in #13657. Let me know what you think about it. Basically, a user not providing it's custom theme shouldn't see a warning. He is doing nothing wrong?

Copy link
Member Author

@oliviertassinari oliviertassinari Nov 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was it introduced in this pull request? Hum, probably because I want to minimize the numbers of commits I'm doing, I try to batch them so our contribution graph looks more balanced. Or maybe because I have realized the issue doing this effort and it was simpler to do it all at once. I should have asked first.

Copy link
Member Author

@oliviertassinari oliviertassinari Nov 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, that was it. I was probably running the benchmark test suite, without providing a custom them then I saw this warning pop. I think that its important people can use Material-UI without having to provide a theme.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to change the tradeoff?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants