-
-
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
[core] Optimise destructuring for useState, useReducer #16842
[core] Optimise destructuring for useState, useReducer #16842
Conversation
@material-ui/core: parsed: -0.29% 😍, gzip: -0.39% 😍 Details of bundle changes.Comparing: c5b518b...d33e0b7
|
Hmm, surely this can't be right? How can the parsed size be exactly the same and the gzip size increase?
|
packages/babel-plugin-transform-destructuring-fork/src/index.js
Outdated
Show resolved
Hide resolved
packages/babel-plugin-transform-destructuring-fork/src/index.js
Outdated
Show resolved
Hide resolved
Like different chunking of webpack. Don't bother with every byte fluctuation. |
@eps1lon that makes sense. What's your thoughts on the approach in this PR though? 🙂 |
It would be nice to add some tests to the babel plugin which have input/output code. I'm a bit surprised this lowered parsed and gzip size. |
No guarantees as to when I'll be able to get to that, but just curious if you've taken a look at the bundle size diff? The reason |
Well, yes
and yes: |
Here's the diff generated from your PR: https://github.com/NMinhNguyen/material-ui/commit/7682e1809a596c7778583be9a07143735bde2cbf At first glance it's actually hard to tell whether |
Most of the time code that minifies better increases gzip size since both use similar compression methods. Again: Include tests (acts as documentation) for input/output and explain what you do differently than babel-plugin-optimize-react and why. IMO this should be a standalone plugin that is subject to peer review outside of the material-ui bubble since it is concerned with react not material-ui specifically. Unless I'm missing something. |
Strong 👍 |
I have commented on this PR as well as in the code itself that I copied
Although I do agree with the idea, this has been added as a private package so we can iterate quickly. UpdateI have now added a README and tests, hopefully this addresses your concerns. |
You added this later without mentioning it. I'm not checking every comment (especially the opening description) in a thread when the thread is updated. Blame github but not me.
But this is not an incubator for babel plugins. I appreciate your work and I feel some sense of pride that that our current size tracking solution is useful for quick iterations. But why did you not propose this for the original plugin? The original authors are better equipped in reviewing this. I guess you used a draft to indicate that this is just a showcase and not meant to be merged? Basically to make the case for this change when proposing it to the original plugin? |
My apologies, I assumed that I had updated the opening description before you got the chance to review this PR. Although I'm pretty confident those individual comments on the code were there shortly after I raised the PR.
Because there is already babel/babel#9486 that has been there since 10 February. I don't think it actually works though, but I did lift the regexes used in that PR and with some debugging of
I used it as a draft to share an idea with the team and to get feedback on the approach. And hopefully to get it merged (after some refinement perhaps?) and eventually replaced by I've spoken to @oliviertassinari offline to avoid back and forth and this is what he proposed:
Point 3 is what I'm gonna verify now. Although in this PR, I made sure that I wasn't including this plugin where it wasn't being included before - it was only being included as part of |
I've verified your Babel configuration and can confirm that this plugin is added as long as In other words, it's included in all cases apart from when you output |
The main downside to manually including this plugin is that prior to this PR, this plugin would be added by Furthermore, |
I like that approach. We used to have it for a react docgen issue. It could be the cleanest option. @eps1lon What do you think? Also, I really like the bundle size reduction on the small universal modules, like the -20% on useMediaQuery. |
Applying unofficial patches was only every used temporary and never? on master. Unless these patches are reviewed by original maintainers they become a maintenance hazard. Maybe babel has to fix a bug tomorrow that creates a merge conflict with this patch? Maybe this patch introduces a subtle bug. Maybe it has a performance impact. There are just dozens of potential issues inherent to changes to the build setup that are just not worth it saving 1kB in a 300kB bundle. I still don't understand how this is material-ui specific? Are we using useState or useReducer differently? Please file a PR upstream, if nothing happens publish the fork so that it is subject to peer review, file a PR applying the published package and this is likely to get applied. See |
I struggle to see how this is different in principle to e.g. you overriding Lastly, if this for some reason doesn’t work out (which I don’t think is the case here given the tests and how simple the change is), you could just revert this commit and keep innovating? |
@eps1lon I get all of your arguments and they make sense from a maintainers perspective but |
One is patching existing code without running that patch through the tests of the original code. The other is using public APIs.
By that argument we should merge anything and "just" revert if it doesn't work out.
Well then I should ask this question: How big are the TTI and load-time performance increases? You could start by measuring the deploy preview.
That was not my assertion but a measurement taken. What do you mean by "off". It's 1052 bytes. I took the liberty and rounded that to 1kB. |
Make me think of our decision making process. There is some truth in that. I think that for a decision that is easily reversible and looks good on paper, we should explore them without hesitation (meaning, take them as fast as we can). What does look good on paper mean? Well, I think that it has to look worth our time, a good positive ROI compared to how we would spend our time otherwise. I don't want to influence this issue, I want to only cover an aspect on how I think that we should make decisions on Material-UI. |
Compromise proposal:
Good: Both at the same time introduces slicedToArray:
I'm going to work on fixing |
I think you're extending the argument too far. I would argue that in the presence of factors such as
this change is actually not that risky. There have been far riskier changes that have been merged in the history of this project.
Also you're still yet to comment on the actual code change being applied to the destructuring plugin. |
The scope was the destructuring. You're getting a bit too eager nitpicking my arguments. |
@eps1lon Are you able to comment on the actual code change itself and what your concerns are exactly? Are you not interested in me walking you through the code change as well? |
I didn't even look at the code changes (since marked as Draft). All of my concerns are with the process itself. The implementation is probably fine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a public repository for this package https://www.npmjs.com/package/@minh.nguyen/plugin-transform-destructuring?
@oliviertassinari not yet, see #16842 (comment) |
Ok, cool, we can wait a week or two. I don't think that we need to rush the changes. |
@oliviertassinari done https://github.com/NMinhNguyen/babel-plugin-transform-destructuring (and PR description updated with a link) |
@NMinhNguyen Thanks, I have submitted a pull request to the repository. I have noticed that you don't watch it, nor that the project has an issues tab enabled nor that the CI is setup. If you have no intention to keep it up to date with
|
@oliviertassinari thanks. It wasn’t intentional - I simply forked Babel’s repo (preserving the Git history for the plugin). It’s my first proper public repo so I’m not familiar with the set up, I was just trying to do the bare minimum that you requested (create a repo which I did and thought was sufficient). I’ll enable issues and review your PR. That being said, I don’t care too much whether you wanna use the published fork or patch-package. I’m fine with either approach, just let me know and I’ll amend my PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I have created a personal reminder for checking babel-plugin-transform-destructuring every couple of months. Let's hope this patch is only temporary :). We will revisit the approach in the future if it causes issues.
@NMinhNguyen Thanks |
Update
GitHub repo now available at https://github.com/NMinhNguyen/babel-plugin-transform-destructuring
I used babel/babel#9486 as an inspiration which was originally intended for Create React App (facebook/create-react-app#5602 (comment)). You can actually see
selectiveLoose
option used in CRA's Babel config although I don't think it does anything as the Babel PR isn't merged.This PR uses patch-package which you may not like as maintainers, but I'm mostly just raising this PR to look at the bundle size diff 😅.When I built packages using #16072 (as well as in my own project at work), I observed that it doesn't seem to handle array destructuring, thus this PR.This PR uses my fork (source on unpkg.com) of
@babel/plugin-transform-destructuring
with the following diff:Show diff
Bundle size diff can be viewed here. An example is provided below for convenience:
I think this PR could be merged and then if (or more like when)
babel-plugin-optimize-react
is ready at some point in the future, this can be replaced with it.