performance: use lazy loading for components #12739
Conversation
nhsz
requested review from
wackerow,
corwintines,
pettinarip and
minimalsm
as code owners
github-actions
bot
added
content 🖋️
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
github-actions
bot
added
content 🖋️
There was a problem hiding this comment.
Lgtm! Loading as expected, maybe just a slight delay on the first load if you go straight to it, but I'm not worried about that given it's location on the page. Nice job!
Wondering if we should just make a helper function for this to lazy load with {ssr: false}
There was a problem hiding this comment.
As discussed offline, I still think there is no good reason to lazily load content components from this list at this early stage of the epic.
By content components I'm referring to CommunityEvents and CalloutBanner. The contents of these components are visible on the first render (they are included in the html file). If we lazy load them, the content will not be visible to crawlers, which is one of the main reasons we do static rendering of this page.
Also, these components do not have heavy deps. On the contrary, they seem to be pretty small units of code. So I wouldn't expect much difference in perf by including them.
The CodeModal & Codeblock are good examples of two components that are great candidates for lazy loading, since they are not visible on the first render and may have other external deps.
This reverts commit c8983f7.
…omepage-lazy-loading
nhsz
changed the title
There was a problem hiding this comment.
Looks good! Will let @pettinarip or @corwintines take another peak, but I think we can get this in for this weeks deploy
There was a problem hiding this comment.
@nhsz from the code side, it looks good, but don't really know how this is impacting the overall performance. Have you done any analysis? or do you have a bundle size comparison?
There was a problem hiding this comment.
@nhsz I've ran some tests on this with the bundle analyzer and compare them with dev.
I've added the results here:
https://docs.google.com/spreadsheets/d/1AKS_h0_xGxyI6L4X9TexJ42IFLkxw6Kc3vgkw5Celfc/edit?usp=sharing
The main _app file is reduced by ~5kb, which is less than 1% improvement for this bundle (the main source of problems). In addition, these changes create 10 more chunks, which is neither good nor bad, but they add more network requests and redundant payload to each chunk, making the total bundle size even larger than the previous one.
I'm not against using lazy load, but I would use it with caution since we might be adding complexity for no good reason.
For example, there are a few components (Tooltip, GlossaryDefinition) in this list that are less than 4kb uncompressed. So creating a new chunk for them might not seem worthwhile.
IMO there is no significant difference in the bundle sizes. The optimization in general doesn't justify adding more complexity to the codebase.
Not sure the next steps here but lmk your thoughts.
|
Closing this out per @pettinarip's comment and the internal discussion around this change. Please re-open if you disagree with my understanding here @nhsz |
github-actions
bot
added
the
abandoned
This PR applies lazy-loading for the following components, to reduce initial JS payload and reduce loading time
CodeblockCodeModalFeedbackCardFeedbackWidgetGlossaryDefinitionGlossaryTooltipModalSimulatorModalTooltip