-
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
Editor: Introduce Gutenberg in Calypso behind feature flag #44801
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Webpack Runtime (~156 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 (~42703 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~1042822 bytes added 📈 [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 (~7774 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. |
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.
WordPress Desktop CI Failure for job "wp-desktop-source".
@saramarcondes please inspect this job's build steps for breaking changes at this link. For temporal failures, you may try to "Rerun Workflow from Failed".
Please also ensure this branch is rebased off latest Calypso.
👋 Hi, @saramarcondes! Could you rebase this PR on latest Calypso master? We merged some fixes into master that should mitigate the out-of-memory |
2410e21
to
b585f0a
Compare
FWIW, I have some notes lying around locally on how to get a server-less Gutenberg (Site Editor) to work which I hope to put in writing later this week 🙂 (Specifically related to |
307b8f0
to
f772f94
Compare
Lovely work! I might borrow from it to improve the Gutenberg playground waiting for some unrelated fixes in #44153 |
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.
Very nice work! Styles will be a great challenge! But other than that the editor is running pretty smoothly and the experience feels much better than with the iframe.
<BlockEditorProvider | ||
value={ blocks } | ||
onInput={ updateBlocks } | ||
onChange={ updateBlocks } |
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 seems that the inserter crash is happening because the block editor is expecting some settings to exist, but they don't exist in Calypso. I don't know if it makes sense having those settings in Calypso, but an easy solution is just passing them to the BlockEditorProvider
component:
onChange={ updateBlocks } | |
onChange={ updateBlocks } | |
settings={ { | |
__experimentalBlockPatternCategories: [], | |
__experimentalBlockPatterns: [], | |
} } |
But it looks like a bug on Gutenberg. It shouldn't be expecting those settings to be set, especially since they're experimental settings. I'll propose a change there.
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 @diegohaz! That fixed it perfectly!
6a054c8
to
ff68d32
Compare
function Gutenberg( props ) { | ||
const { postId, siteId, postType } = props; | ||
|
||
setCurrentSiteId( siteId ); |
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.
/cc @ockham
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.
WordPress Desktop CI Failure for job "wp-desktop-mac".
@saramarcondes please inspect this job's build steps for breaking changes at this link. For temporal failures, you may try to "Rerun Workflow from Failed".
Please also ensure this branch is rebased off latest Calypso.
ff68d32
to
4b99ec2
Compare
wp-desktop ci passing, closing review
@sgomes ccing you on this PR, if you have time I'd love some insight into the performance aspect of what I'm doing here. Is there anything else I should take into consideration that I haven't already? |
@sgomes Thanks for the help. I'm very confused why Gutenboarding is affected at all, I haven't done anything in that area. Is it because I'm importing new |
Sorry for the late reply, @saramarcondes! I've been AFK for the past week.
Interesting. At first glance, I'd say this is likely either due to webpack heuristics or some newly-added package duplication. I'll investigate 👍 |
@sgomes The answer here is probably the same as in a similar issue in PR that adds WordPress Components Gallery to devdocs: the modules shipped in Gutenboarding are still exactly the same, but as many of them are now used in multiple chunks, opportunities for module concatenation optimization disappeared: #45523 (comment) |
Thanks, @jsnajdr, that makes sense and is a very likely cause! 👍 I'll give this a quick look-over to see if I can make sure that's the case. |
Looking at
@scinos Any idea what this could mean? |
On the other hand, 12% size increase is big. The component gallery PR caused increase only on 1% scale. Maybe there are other issues there. |
I just pushed a change that deduplicates |
I would guess that's the result of a bad merge. In theory that shouldn't affect bundling. |
Deduplicating |
Right, I think I understand what's going on. This appears to be down to the definition of a module, and the fact that across a build, it can only have a single definition. In essence, the issue is due to tree-shaking. Previously, Gutenboarding was able to tree-shake some things in To be clear, this is ordinarily not an issue, with each thing ending up in its own section, as needed; leaf modules get moved to where they're needed. However, due to the way these particular This could possibly be fixed or at least mitigated with some more aggressive side-effect handling on |
At this point, I'd consider trying out the new Gutenberg-in-Calypso section not as a separate entrypoint, but rather as a separate application / build altogether. It will definitely be larger, by not sharing anything with the Calypso builds, but the tradeoff of keeping unnecessary stuff out of Gutenboarding could be worth it. The other option, of course, is to just accept the increase and live with it. In the context of an unproven experiment, though, that becomes a bit harder to justify 😕 |
c5f353e
to
7be44e2
Compare
Changes proposed in this Pull Request
This will allow us to more easily test ongoing work to fix Calypso's global styles. By checking for specific things that need to be fixed, we may be able to better prioritize somethings over others (like Buttons may not be necessary to fix immediately as they seem to be okay, but the FormRadio and FormCheckbox do need to be fixed).
Ongoing discussion here p9oQ9f-iN-p2.
AB test
The AB test will always bucket people into the "off" variant. This is correct and desired because we want to be able to easily test Gutenberg in Calypso as we make new style changes without introducing changes to the front-end.
Testing instructions
Fixes #