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

[Table] Use makeStyles over withStyles #15023

Closed

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Mar 23, 2019

This effort is related to #10778. I have benchmarked the following (in production mode):

Performance client-side (+15%)

  • Table Raw:
    Capture d’écran 2019-03-23 à 12 24 48
  • Table Before:
    Capture d’écran 2019-03-23 à 12 27 13
  • Table After:
    Capture d’écran 2019-03-23 à 12 24 37

Performance server-side (+10%)

TableRaw x 2,553 ops/sec ±2.01% (189 runs sampled)
-TableMui x 91.41 ops/sec ±3.62% (168 runs sampled)
+TableMui x 105 ops/sec ±0.83% (172 runs sampled)

DX (-1 component noise)

Before
Capture d’écran 2019-03-23 à 18 56 43

After
Capture d’écran 2019-03-23 à 18 54 54

Ideally, I would rather see MuiTableCell than ForwardRef(TableCell) cc @eps1lon

@oliviertassinari oliviertassinari added performance component: table This is the name of the generic UI component, not the React module! labels Mar 23, 2019
@oliviertassinari oliviertassinari changed the title [Table] Use makeStyles over withStyles [WIP][Table] Use makeStyles over withStyles Mar 23, 2019
@oliviertassinari oliviertassinari force-pushed the table-performance branch 2 times, most recently from a77e578 to 4887ebb Compare March 23, 2019 16:56
@mui-pr-bot
Copy link

mui-pr-bot commented Mar 23, 2019

@material-ui/core: parsed: +0.25% , gzip: +0.57%

Details of bundle changes.

Comparing: 3a8f79a...3946bb4

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core +0.25% 🔺 +0.57% 🔺 350,671 351,564 90,231 90,749
@material-ui/core/Paper +0.03% 🔺 +0.09% 🔺 67,516 67,534 19,830 19,847
@material-ui/core/Paper.esm +0.03% 🔺 +0.07% 🔺 61,114 61,133 18,881 18,895
@material-ui/core/Popper 0.00% +0.02% 🔺 30,463 30,463 10,526 10,528
@material-ui/core/styles/createMuiTheme 0.00% +0.09% 🔺 17,384 17,384 5,724 5,729
@material-ui/core/useMediaQuery 0.00% -0.38% 2,469 2,469 1,043 1,039
@material-ui/lab +0.04% 🔺 +0.03% 🔺 149,576 149,634 44,009 44,022
@material-ui/styles +0.03% 🔺 +0.06% 🔺 52,700 52,716 15,438 15,447
@material-ui/system +0.01% 🔺 -0.24% 17,136 17,138 4,528 4,517
Button +0.02% 🔺 +0.08% 🔺 89,271 89,292 26,506 26,526
Modal +0.02% 🔺 +0.03% 🔺 83,376 83,395 25,007 25,015
colorManipulator 0.00% +0.08% 🔺 3,232 3,232 1,299 1,300
docs.landing 0.00% 0.00% 49,958 49,958 10,837 10,837
docs.main +0.13% 🔺 +0.08% 🔺 646,052 646,905 200,814 200,981
packages/material-ui/build/umd/material-ui.production.min.js +0.17% 🔺 +0.27% 🔺 299,178 299,674 82,966 83,191

Generated by 🚫 dangerJS against 3946bb4

@oliviertassinari oliviertassinari force-pushed the table-performance branch 3 times, most recently from 209795c to e152271 Compare March 23, 2019 17:50
@oliviertassinari oliviertassinari changed the title [WIP][Table] Use makeStyles over withStyles [Table] Use makeStyles over withStyles Mar 23, 2019
@oliviertassinari oliviertassinari added the on hold There is a blocker, we need to wait label Mar 23, 2019
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Ideally, I would rather see MuiTableCell than ForwardRef(TableCell) cc @eps1lon

Agreed. We can probably gather this into a single helper that sets the appropriate statics. Right now this could include:

  • displayName
  • options
  • useStyles

@oliviertassinari
Copy link
Member Author

Agreed. We can probably gather this into a single helper that sets the appropriate statics. Right now this could include:

@eps1lon It's a great idea. For the default props issue. What do you think of duplicating the information? Once in the props destructuring for React and once in this helper for the generation of the documentation?

@oliviertassinari oliviertassinari removed the on hold There is a blocker, we need to wait label Mar 25, 2019
@eps1lon
Copy link
Member

eps1lon commented Mar 25, 2019

Agreed. We can probably gather this into a single helper that sets the appropriate statics. Right now this could include:

@eps1lon It's a great idea. For the default props issue. What do you think of duplicating the information? Once in the props destructuring for React and once in this helper for the generation of the documentation?

Should be good enough as a temporary solution. Seems like a good time to write a custom defaultProps resolver.

@oliviertassinari
Copy link
Member Author

@eps1lon Updated. What do you think of the solution?

packages/material-ui/src/TableFooter/TableFooter.js Outdated Show resolved Hide resolved
@@ -0,0 +1,14 @@
export default function muiComponent(useStyles, Component) {
Copy link
Member

Choose a reason for hiding this comment

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

I guess it makes more sense to use the component as the first parameter. useStyles is more likely to get removed at some point than Component

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't. If I swap the arguments, react-docgen does no longer "see" the component in the file.

Copy link
Member

Choose a reason for hiding this comment

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

This should be addressed in reactjs/react-docgen#343

@jeremy-coleman
Copy link

If using function components, just pass in props with arguments .. function MuiDiv({someKey=someValue, ...props}){}

@jeremy-coleman
Copy link

displayName over rides forwardRef in devtools btw

@joshwooding
Copy link
Member

If using function components, just pass in > props with arguments .. function MuiDiv({someKey=someValue, ...props}){}

We try not to destructure early #15344 (comment)

@oliviertassinari oliviertassinari mentioned this pull request Apr 17, 2019
29 tasks
@merceyz merceyz mentioned this pull request May 7, 2019
1 task
@oliviertassinari oliviertassinari changed the base branch from next to master May 23, 2019 21:09
@oliviertassinari
Copy link
Member Author

I'm closing. To revisit in the future.

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

Successfully merging this pull request may close these issues.

5 participants