-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[theme] Built-in convertLength method #19720
[theme] Built-in convertLength method #19720
Conversation
98a0254
to
96c9301
Compare
Details of bundle changes.Comparing: 0426817...96c9301
|
It all comes down to: What problem are you trying to solve. You haven't answered that. |
In the case of this dependency:
I think that 1. and 3. outweigh 2. It seems to be a win. |
@eps1lon When a dependency solves a simple problem, is not frequently used in the ecosystem, and embodies a problem developers come to Material-UI for, what would be the advantage to keep it? I doubt we should consider the "reduce bundle size" argument per the "not frequently used in the ecosystem" (non-signficant impact, on average). Giving more nuances to the previous arguments around the why:
|
This is a continuation of #19638, one less dependency, among our long list: https://npm.anvaka.com/#/view/2d/%2540material-ui%252Fcore, with the CSS-in-JS module as a peer dependency in v5? It should be much better.
I think it's a good opportunity to continue the reflection on the best dependency policy Material-UI could follow. I think that we should be very strict in the way we accept dependencies. If we go back to the fundamentals, a dependency is about solving a problem we really need to and that we don't want to solve ourselves.
While Vuetify, ag-grid, Lodash, Vue, Bootstrap, or Angular follow a zero direct dependency policy (but accept peer dependencies), I wonder if it's not too strict.
Here are the dimension I think that we should consider in the equation that should increase the chance for adding a dependency:
If a dependency doesn't rank well within these dimensions, it's a good signal we should remove it, hence this pull request.