-
Notifications
You must be signed in to change notification settings - Fork 507
chore: enable react-compiler babel plugin #6107
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
base: master
Are you sure you want to change the base?
Conversation
227f0d9
to
d629382
Compare
😢
|
I tried adding the compiler to babel and our build but the size increase was significant. Not sure why it bloated the size of everything. It claims to optimize the runtime performance and not necessarily the bundle size. We can still use the eslint rule to catch things but not use the babel plugin |
f5e1455
to
1529a99
Compare
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 @bryceosterhaus . Yeah the eslint rule will still be helpful even if we don't use the compiler itself.
Do you happen to know why those packages are so big? I'm surprised that there are any packages larger than core.
@ethib137 yeah that is odd that those are so big. I don't know if the "stats" github action bundlers all the dependencies as well, so it's possible that "card" is also including "core". I will look into it today though and see if we can isolate the package size to just the package itself without the dependencies |
@ethib137 so the reason the sizes are so big is because we are treating each package as an "entry point" so if an existing project brought in We may want to change this to just be the size of the package code itself, OR we explicitly exclude |
Oh interesting! If we explicitly exclude @clayui/* dependencies I wonder if that would change the calculations for using react compiler at all? |
@ethib137 it does significantly change the sizes when excluding clayui/* I am gonna try to revamp the stats a bit and fall more in line with our DXP size report too PR for stats: #6109 |
1529a99
to
90d046b
Compare
testing again with compiler enabled at build to see new stats table |
This is the new size with react-compiler enabled. I believe it is adding size because it is "auto-memoizing" things, so we can expect more code but it should be more performant. I am curious if by enabling this we could remove a bunch of
|
f6c0a34
to
7b5650d
Compare
One interesting note with the compiler enabled, is that I noticed it removed "useMemo" from some of the packages I looked at the built file. |
That is interesting. It's a huge difference in size though, so until we can show evidence that there are sizeable performance improvements, we should probably avoid using the compiler. |
I am going to send a separate PR just for the eslint part and merge it(#6111). I want to keep this up for info on the babel part and we can revisit once we know how to check the performance gains. |
Oh you know what... I think the bloat in every package is due to bundling |
Oh if react-compiler-runtime is bundled that would account for it. Yeah... let's see what else you can find out. |
193d630
to
053c0f0
Compare
053c0f0
to
1975f17
Compare
It looks a little better but still seeing bloated packages. We just need to find a way to appropriately test performance
|
This enables the rule and then also suppresses the few instances where it was erroring. All the cases it found didn't seem super problematic, so we can just suppress them for now.
https://liferay.atlassian.net/browse/LPD-26040