Skip to content

[core] Start the migration to makeStyles - #16146

Closed
merceyz wants to merge 4 commits into
mui:masterfrom
merceyz:core/component-name
Closed

[core] Start the migration to makeStyles#16146
merceyz wants to merge 4 commits into
mui:masterfrom
merceyz:core/component-name

Conversation

@merceyz

@merceyz merceyz commented Jun 10, 2019

Copy link
Copy Markdown
Collaborator

Backstory
In #15023 @oliviertassinari started migrating Table to use the new makeStyles API. For the tests and documentation to continue to work he made a muiComponent helper. That function allowed the tests to get the useStyles instance, and the docs to get the className and styles used. However the downside of this was overhead at runtime and more code.

Goal
The goal of this PR is to use a babel plugin to inject the information needed by the tests and docs without the runtime overhead. And then migrate the components to makeStyles

Note
Title is a work in progress, edits welcome

Todo

  • export muiDisplayName
  • export muiStylesOrCreator
  • Inject name in makeStyles automatically
  • Start the migration to makeStyles

Comment thread packages/babel-plugin-set-component-name/src/index.js Outdated
@merceyz

merceyz commented Jun 10, 2019

Copy link
Copy Markdown
Collaborator Author

@mui-pr-bot

mui-pr-bot commented Jun 10, 2019

Copy link
Copy Markdown
Details of bundle changes.

Comparing: 85058cc...246ef62

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core 0.00% 0.00% 318,556 318,556 87,551 87,551
@material-ui/core/Paper 0.00% 0.00% 68,345 68,345 20,371 20,371
@material-ui/core/Paper.esm 0.00% 0.00% 61,639 61,639 19,173 19,173
@material-ui/core/Popper 0.00% 0.00% 28,966 28,966 10,415 10,415
@material-ui/core/Textarea 0.00% 0.00% 5,513 5,513 2,370 2,370
@material-ui/core/TrapFocus 0.00% 0.00% 3,755 3,755 1,581 1,581
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 16,016 16,016 5,821 5,821
@material-ui/core/useMediaQuery 0.00% 0.00% 2,529 2,529 1,088 1,088
@material-ui/lab 0.00% 0.00% 138,981 138,981 42,767 42,767
@material-ui/styles 0.00% 0.00% 51,765 51,765 15,346 15,346
@material-ui/system 0.00% 0.00% 14,825 14,825 4,241 4,241
Button 0.00% 0.00% 84,274 84,274 25,679 25,679
Modal 0.00% 0.00% 20,343 20,343 6,682 6,682
colorManipulator 0.00% 0.00% 3,904 3,904 1,544 1,544
docs.landing 0.00% 0.00% 55,232 55,232 13,947 13,947
docs.main -0.01% -0.00% 652,812 652,764 206,694 206,689
packages/material-ui/build/umd/material-ui.production.min.js 0.00% 0.00% 291,991 291,991 83,425 83,425

Generated by 🚫 dangerJS against 246ef62

@joshwooding

joshwooding commented Jun 10, 2019

Copy link
Copy Markdown
Collaborator

Just for completion sakes, I'm using the experimental react dev tools and it looks like this in dev mode:
image

Production:
Before:
image

After the changes:
image

@merceyz
merceyz force-pushed the core/component-name branch from 2096f45 to 2156eb9 Compare June 10, 2019 22:33
@merceyz

merceyz commented Jun 10, 2019

Copy link
Copy Markdown
Collaborator Author

@joshwooding Going to disable the displayName part in production unless it's something that is wanted

@eps1lon

eps1lon commented Jun 11, 2019

Copy link
Copy Markdown
Member

Have a look at babel-plugin-unwrap-createStyles on how to integrate in-repo babel plugins:

https://github.com/mui-org/material-ui/blob/07b9d6efaac1b663b2657173227e9490c796191e/package.json#L91

@merceyz

merceyz commented Jun 11, 2019

Copy link
Copy Markdown
Collaborator Author

@eps1lon I can't figure out why the dangerjs command is failing, any suggestions?

@oliviertassinari

oliviertassinari commented Jun 11, 2019

Copy link
Copy Markdown
Member

@joshwooding Thanks for looking at the experimental React Dev Tool output for this change. Big +1 for optimizing for the upcoming version, rather than the current one.

Comment thread babel.config.js Outdated
@merceyz
merceyz force-pushed the core/component-name branch from 643eccc to 0cfc9ce Compare June 11, 2019 12:44
@eps1lon

eps1lon commented Jun 11, 2019

Copy link
Copy Markdown
Member

I think we shouldn't hide implementation details with display names. Implementation details get leaked anyway via devtools or react component stack traces. It's tempting to simplify it but in the end it might confuse even more people if they get stack traces that seem like circular references:

some error
in Typography
in ForwardRef(Typography)
in Typography

instead of

some error
in WithStyles(ForwardRef(Typography))
in ForwardRef(Typography)
in Typography

And react-devtools will be improved anyway as was already mentioned

@merceyz
merceyz force-pushed the core/component-name branch from 0cfc9ce to da544e8 Compare June 11, 2019 15:42
@merceyz

merceyz commented Jun 11, 2019

Copy link
Copy Markdown
Collaborator Author

@eps1lon True, I'll continue to work on this so the plugin does the job of muiComponent in #15023.
So that the transition to makeStyles is as simple as possible, without the runtime overhead of muiComponent

@merceyz
merceyz force-pushed the core/component-name branch 4 times, most recently from 3aa1064 to 98f9353 Compare June 11, 2019 16:57
@merceyz

merceyz commented Jun 11, 2019

Copy link
Copy Markdown
Collaborator Author

@oliviertassinari Is it on purpose that Modal doesn't have any documentation for classes?

@oliviertassinari

oliviertassinari commented Jun 11, 2019

Copy link
Copy Markdown
Member

Is it on purpose that Modal doesn't have any documentation for classes?

@merceyz Yes, there is no class names. It's using inline-style.

I don't understand the objective of the pull request. Could you update the description so everybody can follow what we change and why :)? Thanks!

@merceyz

merceyz commented Jun 11, 2019

Copy link
Copy Markdown
Collaborator Author

Yes, there is no class names. It's using inline-style.

Ah, I see, Thanks

I don't understand the objective of the pull request. Could you update the description so everybody can follow what we change and why?

Sure, updated.

@merceyz
merceyz force-pushed the core/component-name branch from 3d45e61 to b4beaab Compare June 11, 2019 18:22
@oliviertassinari

oliviertassinari commented Jun 11, 2019

Copy link
Copy Markdown
Member

@merceyz Thanks for providing more details! #15023 was closed because we might not go into this direction in the future. I think that we should explore a style adapter base strategy, a world where we can provide a naked, styled-components, JSS, linaria, etc. version of the components while using styled-components as the default style adapter and maintaining JSS for backward compatibility.

The value here is that we can defuse the fears people have, too many people are building their design system from scratch. It's wasteful.

  • Will it bloat my application? No, you can use the naked components if you are courageous enough to restyle everything (only large enterprise will do that) or use the styling solution that has the best tradeoff for your case.
  • Will It be faster for me to customize the component to match the company design or write it from scratch? We need to make sure it's faster to customize our components! Good luck getting accessibility, documentation and all the features right.

@merceyz

merceyz commented Jun 11, 2019

Copy link
Copy Markdown
Collaborator Author

@oliviertassinari I'm aware, seems like a great idea. Would that change what makeStyles uses under the hood or is the idea to change what is used on the surface(in the components) as well?

The plan with this PR is just to make it easier to migrate away from withStyles

@oliviertassinari

oliviertassinari commented Jun 11, 2019

Copy link
Copy Markdown
Member

@merceyz There are many challenges, one is with the API we expose. How do we minimize the amount of work needed? The advantage of the HOC is that we can take the naked components and wrap it. I will look for precedence in the industry. So far, I have seen two libraries exploring this path:

I'm excited about it!

@merceyz

merceyz commented Jun 11, 2019

Copy link
Copy Markdown
Collaborator Author

The advantage of the HOC is that we can take the naked components and wrap it

Same can be done with a hook, use a context to provide the style engine, fallback to JSS.
Some option to disable the fallback and you have the naked component

@merceyz merceyz changed the title [core] Automatically set component name [core] Migrate to makeStyles Jun 11, 2019
@oliviertassinari

Copy link
Copy Markdown
Member

Same can be done with a hook, use a context to provide the style engine, fallback to JSS.
Some option to disable the fallback and you have the naked component

@merceyz I don't see how this can work. I think that we should hold any migration effort so we can POC around different style adapter approaches. We will have a better understanding of what's important once we have evaluated all our options and found the best tradeoff. I think that we will be more efficient by not moving from withStyles to makeStyles until this point. What do you think?

@merceyz
merceyz force-pushed the core/component-name branch from 246ef62 to b4beaab Compare June 11, 2019 20:21
@joshwooding

joshwooding commented Jun 11, 2019

Copy link
Copy Markdown
Collaborator

I don't think the migration should happen in this PR either way. Sounds like a lot of change for one PR

@merceyz merceyz changed the title [core] Migrate to makeStyles [core] Start the migration to makeStyles Jun 11, 2019
@merceyz

merceyz commented Jun 11, 2019

Copy link
Copy Markdown
Collaborator Author

I think that we will be more efficient by not moving from withStyles to makeStyles until this point. What do you think?

Could make the plugin do all the work (migration) but yeah, should probably hold off until a decision has been made.

@merceyz merceyz closed this Jun 11, 2019
@oliviertassinari

Copy link
Copy Markdown
Member

Alright, let's explore :D!

@merceyz
merceyz deleted the core/component-name branch June 14, 2019 10:19
@zannager zannager added the internal Behind-the-scenes enhancement. Formerly called “core”. label Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal Behind-the-scenes enhancement. Formerly called “core”.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants