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

[styles] Go back to index counter #15044

Merged
merged 1 commit into from
Mar 25, 2019

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Mar 25, 2019

One slice of #14999.

This has different advantages:

  • Runtime speed-up: +10%. We spend around 10% (62.9 ms / 637 ms) of the time generating the hashes.

Capture d’écran 2019-03-25 à 10 02 59

  • It improves the overriding story of the library. The hash generation wasn't unique. We were injecting duplicates.

Capture d’écran 2019-03-25 à 09 55 34
https://next.material-ui.com/demos/buttons/#buttons

  • It reduces the bundle size. We can save the import for @emotion/hash.

The hash logic was added in the first place to handle server-side sheets caching. I have kept the hash version of the class name generator. Server-side caching is still a beta feature. It's gonna need more R&D. cc @kof

@oliviertassinari oliviertassinari added performance package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. labels Mar 25, 2019
@@ -1,17 +1,11 @@
import warning from 'warning';
import hash from '@emotion/hash';

const escapeRegex = /([[\].#*$><+~=|^:(),"'`\s])/g;
Copy link
Member Author

Choose a reason for hiding this comment

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

@mui-pr-bot
Copy link

mui-pr-bot commented Mar 25, 2019

@material-ui/core: parsed: -0.26% 😍, gzip: -0.40% 😍
@material-ui/lab: parsed: -0.61% 😍, gzip: -0.77% 😍
@material-ui/styles: parsed: -1.73% 😍, gzip: -2.05% 😍

Details of bundle changes.

Comparing: 3a8f79a...e77d465

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core -0.26% -0.40% 350,671 349,760 90,231 89,867
@material-ui/core/Paper -1.35% -1.58% 67,516 66,605 19,830 19,516
@material-ui/core/Paper.esm -1.49% -1.66% 61,114 60,203 18,881 18,567
@material-ui/core/Popper 0.00% 0.00% 30,463 30,463 10,526 10,526
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 17,384 17,384 5,724 5,724
@material-ui/core/useMediaQuery 0.00% 0.00% 2,469 2,469 1,043 1,043
@material-ui/lab -0.61% -0.77% 149,576 148,665 44,009 43,670
@material-ui/styles -1.73% -2.05% 52,700 51,789 15,438 15,122
@material-ui/system 0.00% 0.00% 17,136 17,136 4,528 4,528
Button -1.02% -1.28% 89,271 88,360 26,506 26,166
Modal -1.09% -1.40% 83,376 82,465 25,007 24,657
colorManipulator 0.00% 0.00% 3,232 3,232 1,299 1,299
docs.landing 0.00% 0.00% 49,958 49,958 10,837 10,837
docs.main -0.16% -0.17% 646,052 645,032 200,814 200,470
packages/material-ui/build/umd/material-ui.production.min.js -0.31% -0.40% 299,178 298,253 82,966 82,632

Generated by 🚫 dangerJS against e77d465

@kof
Copy link
Contributor

kof commented Mar 25, 2019

Glad you did this. JSS used hashes long time ago too, then I tried to optimize it this way too. I think I have now some ideas on how to avoid problems with mismatching class names after SSR by providing meaningful feedback to the user and documenting some edge cases. Once this is solved we are really good here!

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Mar 25, 2019

  • Based on the survey, most people do client-side rendering only. I would rather optimize for them first, then for the server-side users.
  • The index counter is causing a lot of configuration trouble for SSR users. I hope that the new API [styles] Server-side rendering API #15030 will help.
  • I also think that we should make dev & prod class name output identical. It's soo frustrating to have something working in dev mode, to later see it broken in prod. I would rather make the compression optional.

@kof
Copy link
Contributor

kof commented Mar 25, 2019

I also think that we should make dev & prod class name output identical. It's soo frustrating to have something working in dev mode, to later see it broken in prod. I would rather make the compression optional.

Compression off by default or just allow to opt-out?

@oliviertassinari
Copy link
Member Author

off by default.

@thevarunraja
Copy link

If the page has multiple react trees, the index counter breaks the css. Is there any way to handle this scenario? The hash version works perfectly with multiple react trees.

@oliviertassinari
Copy link
Member Author

@thevarunraja Have you tried the seed option? https://next.material-ui.com/css-in-js/api/#creategenerateclassname-options-class-name-generator

@oliviertassinari oliviertassinari merged commit 49e97e3 into mui:next Mar 25, 2019
@oliviertassinari oliviertassinari deleted the styles-index-counter branch March 25, 2019 21:43
@ellisio ellisio mentioned this pull request Apr 16, 2019
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants