-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Devdocs: WordPress components gallery #45523
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Webpack Runtime (~126 bytes added 📈 [gzipped])
Webpack runtime for loading modules. It is included in the HTML page as an inline script. Is downloaded and parsed every time the app is loaded. App Entrypoints (~8290 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~2194 bytes removed 📉 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~213860 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
196a842
to
5998205
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.
Great work @saramarcondes!
I think we should ship this as soon as possible, and it will help inform our decisions for any future work, related to global style migrations. Any styling problems or visual glitches should be subject to future work. We can also iterate on the particular examples in future PRs as well.
I've left some nits here and there, but nothing I feel is particularly blocking.
One thing I wanted to ask you was the bundle size changes - do you have an idea why there are changes to sections that don't seem related? Do we just need to recalculate them?
Again, great to see this in Calypso! 🎉
client/devdocs/design/wordpress-components-gallery/autocomplete.scss
Outdated
Show resolved
Hide resolved
client/devdocs/design/wordpress-components-gallery/base-control.tsx
Outdated
Show resolved
Hide resolved
client/devdocs/design/wordpress-components-gallery/autocomplete.tsx
Outdated
Show resolved
Hide resolved
/* eslint-enable */ | ||
) } | ||
</Autocomplete> | ||
<p>Type ~ for triggering the autocomplete.</p> |
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.
Not sure why I'm unable to make this work by typing ~
😉
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.
Yes, it doesn't seem to work, I think it's potentially one of the things that is broken 😱
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.
Maybe it's something related to how its popover works? But we can investigate in another PR, I don't consider that a blocker 😉
client/devdocs/design/wordpress-components-gallery/angle-picker-control.tsx
Outdated
Show resolved
Hide resolved
@@ -211,7 +211,7 @@ | |||
"@types/wordpress__api-fetch": "^3.2.2", | |||
"@types/wordpress__block-editor": "^2.2.8", | |||
"@types/wordpress__block-library": "^2.6.0", | |||
"@types/wordpress__components": "^8.5.4", | |||
"@types/wordpress__components": "^9.8.0", |
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.
Any concern from the gutenboarding point of view from updating this? Should we test anything before shipping? cc @Automattic/luna
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.
I think a quick smoke test is to see if anything breaks on /new
. Current link: https://hash-829c18b8bcfddd5cf96dd3c4bfb03dd0b6df309f.calypso.live/new
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.
Type issues should fail loudly in CI anyway, no?
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.
It shouldn't be necessary to smoke test anything given it's just type definition updates right? The @wordpress/components
package itself did not get upgraded.
I have no idea why the bundle size changed for Gutenboarding in this PR. The only thing I can think is that maybe because we're importing all the WP components that some tree-shaking isn't working as expected anymore? Maybe @sgomes would have some insight for how to investigate. |
Putting everything behind async load marginally improved some things but increased the manifest size by a similar amount that it reduced the Gutenboarding size... and it didn't even fix the problem with Gutenboarding getting bigger. I don't think this is the right solution. |
Yeah, I'd doubt that would be a good solution. Components would generally end up being in the |
I did some research about the The entire The I think the entire growth can be attributed to the module concatenation optimization being less efficient after the WordPress comonent gallery is added. For a module to be concatenate-able, it must be used only once, and not referenced from any other chunk. It's just like inlining functions -- there can't be a pointer to an inline function, and using a pointer to a function prevents it to be optimized by inlining. With the WordPress component gallery, many components that were until now used in only one chunk are now used in two. Therefore preventing their concatenation. I see two solutions:
|
Thanks for clear explanation @jsnajdr !
I'd be fine with this (cc @Automattic/luna). Considering that we want to move towards using |
Given that I think the "normal" one is preferrable. Thanks for the explanation, Jarda. It is super helpful. |
a33688f
to
5e4584a
Compare
Thanks for the very detailed explanation about the webpack optimization issue @jsnajdr.
I agree that accepting the cost here is fine. The hope is that we’re going to use these components throughout Calypso so they’ll end up being shared by every entrypoint anyway.
This is still probably worth pursuing. devdocs includes many large dependencies that will never be used elsewhere in Calypso. For example, react-live is over 2 MB in size and only useful in the context of devdocs. |
5e4584a
to
61cacf0
Compare
I've rebased this to take advantage of the |
Just like @griffbrad, I agree both of the solutions @jsnajdr suggested make sense - both accepting the trade-offs and considering moving devdocs to either its own entry point or a totally separate app. I'm actually happy to ship this already since it will be very helpful as we're working on the global styles migration and we get to use more and more Does anyone object to shipping it as-is? cc @Automattic/luna @Automattic/team-calypso |
@tyxla I'll leave it to your team to decide, thanks for the ping. 👍 Really looking forward to see where this brings us! |
Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
…e.scss Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
They'll be imported globally by #45724
61cacf0
to
6f63ff4
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.
I agree. Let’s ship it.
Thanks, @saramarcondes, for the brilliant work here! Also thanks everyone for the feedback! Shipping. |
Changes proposed in this Pull Request
Testing instructions
Note that some components are represented through the building of the page itself, for example,
Card
et co as well as theFlex
module were used to build the page, so I didn't feel it was necessary to include them as distinct examples. Actually using them is probably the best way to show that they work.