Skip to content
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

Performance - Lazy load homepage, nav drawer and user menu #1947

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

jonkafton
Copy link
Contributor

@jonkafton jonkafton commented Jan 8, 2025

What are the relevant tickets?

Adresses https://github.com/mitodl/hq/issues/6290

Description (What does it do?)

See comment https://github.com/mitodl/hq/issues/6290#issuecomment-2576079678

Our aim is to improve page load performance by reducing the Total Blocking Time which is not scoring well in PageSpeed Insights.

This change lazy loads all of the homepage sections and lazy loads the header nav drawer and user menu both skipping SSR.

An initial approach was to lazy load parts of the page that are not displayed up front, such as the resource drawer and dialogs so there is no impact to initial layout rendering. In turns out however, perhaps counter intuitively, that lazy loaded components are still server rendered unless set to render only on the client (see Skipping SSR).

What happens is that lazy loaded code is excluded from the initial page JS so that although the component's HTML is pre-rendered, the client side hydration is deferred until the code is fetched and compiled. This unfortunately does not improve the overall TBT, which is measured to Time To Interactive, in turn measured as the point the main thread is free of blocking tasks for 5 seconds, irrespective of DOMContentLoaded and Onload events, however these events arrive some seconds sooner while profiling in Chrome Dev Tools with heavy CPU throttle (x20 slowdown).

The TBT reported when running Lighthouse locally does seem to score significantly better, though these is unreliable given the inconsistent test conditions of a local machine 1. We cannot know how Lighthouse running on the PageSpeed Insights tool will respond without releasing (or setting up an entire environment plus CDN), so this this PR should be treated as an experiment that at very minimum has no adverse impact on performance or user experience. For reference, PageSpeed is currently scoring overall performance at 62, with a TBT well in the red at 2,220 ms.

The main upshot though is that this drastically reduces the size of the First Load JS as shown in the Next.js build stats. On current main:

Route (app)                              Size     First Load JS
┌ ƒ /                                    18.5 kB         499 kB
...
+ First Load JS shared by all            174 kB
  ├ chunks/87c73c54-4e62953253cda143.js  53.1 kB
  ├ chunks/987-b6c97acee2308a2d.js       118 kB
  └ other shared chunks (total)          2.96 kB 

With this change:

Route (app)                              Size     First Load JS
┌ ƒ /                                    4.02 kB         237 kB
...
+ First Load JS shared by all            174 kB
  ├ chunks/87c73c54-4e62953253cda143.js  53.1 kB
  ├ chunks/987-b6c97acee2308a2d.js       118 kB
  └ other shared chunks (total)          3.51 kB

Whether this presents an actual performance improvement though is questionable. The First Load JS is explained as "The size of assets downloaded when visiting the page from the server" 2, ie. not when navigating between routes on the client. In both cases the assets are all downloaded during the initial load sequence. The DOMContentLoaded signal arrives further in advance of Onload, though the request waterfalls are ostensibly very similar overall irrespective of Next.js' demarcation of initial chunks, see below. We are interested in interactivity and not initial DOM render - with SSR the initial layout is already rendered way in advance alongside the First Contentful Paint marker. By lazy loading code we may alleviate the initial work to hydrate the page, though are still deferring interactively (though can be selective in which interactivity). The initial HTML size is the same (though initial server response time is the most significant factor in load event timings) and this branch shows more requests for JS (expected as we split off chunks for lazy loading) and slightly more kB of JS overall (736kB against 710kB):

Main:
image

This branch:
image

Similarly, the main thread activity flame charts and importantly the timings for scripts waiting on the main thread (these are the tails on the script network requests below) are identical:

Main:
image

This branch:
image

How can this be tested?

The homepage and remaining site should load without any deterioration of initial render speed. The nav drawer and user menu should be responsive under normal network conditions.

The review needed here is really to validate the thinking and interpretation of things while we experiment with this approach.

Additional Context

Adds jest-next-dynamic-ts to preload dynamically imported components during tests.

Footnotes

  1. https://github.com/GoogleChrome/lighthouse?tab=readme-ov-file#why-does-the-performance-score-change-so-much

  2. https://nextjs.org/docs/app/api-reference/cli/next#next-build-options

@jonkafton jonkafton marked this pull request as draft January 8, 2025 22:26
@jonkafton jonkafton marked this pull request as ready for review January 10, 2025 15:39
@rhysyngsun rhysyngsun self-assigned this Jan 10, 2025
@jonkafton jonkafton added the Needs Review An open Pull Request that is ready for review label Jan 10, 2025
@jonkafton jonkafton changed the title Performance - Lazy load homepage, nav drawer and use menu Performance - Lazy load homepage, nav drawer and user menu Jan 10, 2025
Copy link
Contributor

@rhysyngsun rhysyngsun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good, but I'm getting this error on your branch but on on main:
Screenshot 2025-01-10 at 14-41-49 Learn with MIT MIT Learn

@rhysyngsun rhysyngsun added Waiting on author and removed Needs Review An open Pull Request that is ready for review labels Jan 10, 2025
@jonkafton jonkafton force-pushed the jk/6290-lazy-load-homepage-sections branch from e594828 to 7c44ecc Compare January 15, 2025 19:17
@jonkafton
Copy link
Contributor Author

Generally looks good, but I'm getting this error on your branch but on on main: Screenshot 2025-01-10 at 14-41-49 Learn with MIT MIT Learn

This was due to a timing issue with the request for the user session (useUserMe) now made before the lazy loaded code, resulting in the action buttons being renderable on first render cycle and there mismatching the server rendered content. Not quite a race condition as the query seems to predictably be made before the code loads, irrespective of response timing. @ChristopherChudzicki and I discussed using suppressHydrationWarning, though this did not work, reasons not clear. The solution was to add a hook to prevent the action buttons from being rendered on the first pass, similar to what is described here: https://react.dev/reference/react-dom/client/hydrateRoot#handling-different-client-and-server-content.

The lazy loading also threw off the prefetch warnings (our mechanism to check that API calls made during page load have been prefetched on the server). Without any obvious is hydrated event, the only option seems to be to retry with delay - not ideal and definitely invite a better suggestion.

@jonkafton jonkafton added Needs Review An open Pull Request that is ready for review and removed Waiting on author labels Jan 15, 2025
@jonkafton jonkafton requested a review from rhysyngsun January 15, 2025 21:31
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got the hydration errors as @rhysyngsun did, and they are fixed after @jonkafton 's updates.

Also:

  • Lighthouse does not show a clear difference locally.
  • ...but as @jonkafton mentioned, DomContentLoaded does fire faster, which means page should be interactive sooner.
    • particularly on slower networks and with uncached JS, since it should fire after the. "First Load JS", which is now smaller. Additional JS loads async after "First Load JS"

Let's merge this and measure on RC. If we don't see a clear difference there, we should consider reverting, as this does add some code complexity.

Comment on lines +18 to +20
return () => {
setMounted(false)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I don't think this actually does anything. If the component unmounts, its state is irrelevant.

Copy link
Contributor

@rhysyngsun rhysyngsun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review An open Pull Request that is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants