evaluate polished #15540
evaluate polished #15540
Conversation
oliviertassinari
force-pushed
the
evaluate-polished
branch
4 times, most recently
from
cc2f67e to
c180843
Compare
oliviertassinari
force-pushed
the
evaluate-polished
branch
from
c180843 to
9eb6a6c
Compare
|
@material-ui/core: parsed: +3.73% , gzip: +3.76% Details of bundle changes.Comparing: 02f135d...bf07138
|
oliviertassinari
force-pushed
the
evaluate-polished
branch
from
360b97a to
fdc0328
Compare
|
I have got my answers. The cost is +3 kB gzipped, |
This comment has been minimized.
This comment has been minimized.
Why not contribute to polished and make it tree shakeable? |
|
@eps1lon Without tree-shaking, we get a bundle size report of +10kB gzipped. With ES5 module direct import, we get a bundle size report of +3kB gzipped. Will tree-shaking support help with the +3kB increase? |
|
It's probably too early to publish |
|
I believe the bundle size different comes from polished supporting features like https://github.com/styled-components/polished/blob/master/src/internalHelpers/_nameToHex.js. |
|
I'm not sure what you mean with no tree-shaking support. It seems to work just fine. The last state of this PR used path imports from the cjs bundle. CommonJS has very limited tree-shaking support. Why not go with the documented import style? |
It was my first attempt, the whole library was included: bf07138.
Definitely. |
|
And how did you come to the conclusion that tree shaking is not working? It looks like polished just has more features. I quickly tried it in a new create-react-app and it didn't pull in the full package. |
The bundle size increase reported was +10kB gzipped, exactly like BundlePhobia does. I assumed it wasn't working. |
|
|
| @@ -3,7 +3,8 @@ | |||
| import React from 'react'; | |||
| import PropTypes from 'prop-types'; | |||
| import clsx from 'clsx'; | |||
| import { fade, withStyles } from '@material-ui/core/styles'; | |||
| import { withStyles } from '@material-ui/core/styles'; | |||
| import { fade } from '@material-ui/core/styles/colorManipulator'; | |||
There was a problem hiding this comment.
@oliviertassinari This will pull in our commonJS build.
RemindMe: Write a lint rule that only allows 1st level path imports
There was a problem hiding this comment.
It deoptimizes it. Thanks!
💯 for an eslint rule. You already mentioned this problem and fixed a couple of issues in the past. But I hadn't fully realized the implications.
There was a problem hiding this comment.
Yeah this adds a lot of confusion. It's not obvious what imports are fine and which are not.
For v5 we should revisit our packaging strategy so that people don't even have to choose how they import from our package.
There was a problem hiding this comment.
For v5 we should revisit our packaging strategy so that people don't even have to choose how they import from our package.
What direction do you have in mind? I wish we were documenting:
import { TextField, makeStyles, Theme, PaperProps } from '@material-ui/core';There was a problem hiding this comment.
Or @material-ui/TextField. Either way I think an option for the import style is helpful. We can revisit this in a separate issue. Before a change I want to give users some time to give feedback in the form of the import style they use. This change should come with a codemod.
There was a problem hiding this comment.
Or @material-ui/TextField.
A past effort: #9532, it was implemented with a codemod. I can't find the other thread about it, we discussed the usage of a package per component. The @material-ui/TextField strategy has important limitations: "dependency hell" being the biggest.
I think that it's also part of the reason why people want us to implement all the component here: so they don't have dozens of dependencies to handle. It's a net plus for DX. But it also has drawbacks, it's definitely a tradeoff. Damn, I wish I can find this thread.
There was a problem hiding this comment.
I have found it. I thought it was more detailed, but it seems it just me arguing against myself: #7839 (comment). To give a proper debate.
|
@oliviertassinari What is the goal with the potential move to polished? I'm happy to help work on some sizing issues if it helps get polished integrated and saves you having to build and maintain your own helpers. Is it just the subset in this PR that you're looking at including? |
|
@bhough The motivation to use polished is to compound the community into a single CSS utility package. Material-UI's mission is to empower developers to build UIs with React. So far, we have a more or less (blurred boundary) private CSS utilities. To truly execute this mission, it's important for us to have an outstanding bundle size cost. People are afraid of bundle size bloat. This is also related to #13039. |
|
@oliviertassinari Makes complete sense. We're working on both adding additional contrast functionality (getting ratio and adjusting it) as well as pulling preset values (like the CSS color names) into optional imports (which should address the majority of the size difference you're seeing). In the spirit of this, we're happy to move 4.0 forward if there is a genuine interest in integrating polished into Material-UI. |
@bhough We want to better align with the React community in Material-UI v5. Depending on the trends in 6 months (we will focus on implementing new components in the next 6 months), It might mean moving to styled components, styled-system and polished. Now, there are a couple of challenges with each of these migrations. The challenge with polished is about bundle size, +3 kB gzipped is a blocker. We are going from 1.4 kB to 4.4 kB gzipped here, something is wrong. For us, It doesn't really matter where the CSS helpers are hosted, as long as the bundle size is awesome and they are well documented. We still don't document the color helpers after 3 years of existence. It has never been really important. The challenge with styled-system is the different theming structure it requires, creating a massive BCs most people can't handle. It's also about whether or not it worth it, the gain is limited. |
I'm curious about the bundle size of https://polished.js.org/ as well as if we can easily migrate to it.