Skip to content

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

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

Closed
wants to merge 11 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions frontends/api/src/ssr/useMounted.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { useEffect, useState } from "react"

/*
* Intended for cases where the client content would otherwise be different
* from the server content on the first render pass in the browser and therefore
* cause a hydration mismatch error. We've seen this for example when lazy loading
* components with next/dynamic, whuch produces a race condition with client only /
* session based API responses.
*
* https://react.dev/reference/react-dom/client/hydrateRoot#handling-different-client-and-server-content
*/
export const useMounted = () => {
const [mounted, setMounted] = useState(false)

useEffect(() => {
setMounted(true)

return () => {
setMounted(false)
}
Comment on lines +18 to +20
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.

}, [])

return mounted
}
4 changes: 3 additions & 1 deletion frontends/api/src/ssr/usePrefetchWarnings.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { renderHook } from "@testing-library/react"
import { renderHook, waitFor } from "@testing-library/react"
import { useQuery } from "@tanstack/react-query"
import { usePrefetchWarnings } from "./usePrefetchWarnings"
import { setupReactQueryTest } from "../hooks/test-utils"
@@ -35,6 +35,7 @@ describe("SSR prefetch warnings", () => {
initialProps: { queryClient },
})

await waitFor(() => expect(console.info).toHaveBeenCalledTimes(1))
expect(console.info).toHaveBeenCalledWith(
"The following queries were requested in first render but not prefetched.",
"If these queries are user-specific, they cannot be prefetched - responses are cached on public CDN.",
@@ -97,6 +98,7 @@ describe("SSR prefetch warnings", () => {
initialProps: { queryClient },
})

await waitFor(() => expect(console.info).toHaveBeenCalledTimes(1))
expect(console.info).toHaveBeenCalledWith(
"The following queries were prefetched on the server but not accessed during initial render.",
"If these queries are no longer in use they should removed from prefetch:",
33 changes: 27 additions & 6 deletions frontends/api/src/ssr/usePrefetchWarnings.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { useEffect } from "react"
import { useEffect, useState } from "react"
import type { Query, QueryClient, QueryKey } from "@tanstack/react-query"
import { useMounted } from "./useMounted"

const logQueries = (...args: [...string[], Query[]]) => {
const queries = args.pop() as Query[]
@@ -17,7 +18,13 @@ const logQueries = (...args: [...string[], Query[]]) => {
)
}

const PREFETCH_EXEMPT_QUERIES = [["userMe"]]
const PREFETCH_EXEMPT_QUERIES = [
["userMe"],
["userLists", "membershipList", "membershipList"],
["learningPaths", "membershipList", "membershipList"],
]

const RETRIES = process.env.JEST_WORKER_ID ? 1 : 10

/**
* Call this as high as possible in render tree to detect query usage on
@@ -39,13 +46,23 @@ export const usePrefetchWarnings = ({
*/
exemptions?: QueryKey[]
}) => {
const mounted = useMounted()
const [count, setCount] = useState(0)
const [potentialWarnings, setPotentialWarnings] = useState(true)

useEffect(() => {
if ((potentialWarnings && count < RETRIES) || count === RETRIES - 1) {
setTimeout(() => setCount(count + 1), 250)
}
}, [count, potentialWarnings])

/**
* NOTE: React renders components top-down, but effects run bottom-up, so
* this effect will run after all child effects.
*/
useEffect(
() => {
if (process.env.NODE_ENV === "production") {
if (process.env.NODE_ENV === "production" || !mounted) {
return
}

@@ -63,7 +80,7 @@ export const usePrefetchWarnings = ({
!query.isDisabled(),
)

if (potentialPrefetches.length > 0) {
if (potentialPrefetches.length > 0 && count === RETRIES) {
logQueries(
"The following queries were requested in first render but not prefetched.",
"If these queries are user-specific, they cannot be prefetched - responses are cached on public CDN.",
@@ -80,17 +97,21 @@ export const usePrefetchWarnings = ({
!query.isDisabled(),
)

if (unusedPrefetches.length > 0) {
if (unusedPrefetches.length > 0 && count === RETRIES) {
logQueries(
"The following queries were prefetched on the server but not accessed during initial render.",
"If these queries are no longer in use they should removed from prefetch:",
unusedPrefetches,
)
}

setPotentialWarnings(
potentialPrefetches.length > 0 || unusedPrefetches.length > 0,
)
},
// We only want to run this on initial render.
// (Aside: queryClient should be a singleton anyway)
// eslint-disable-next-line react-hooks/exhaustive-deps
[],
[mounted, count],
)
}
5 changes: 5 additions & 0 deletions frontends/jest-shared-setup.ts
Original file line number Diff line number Diff line change
@@ -5,6 +5,7 @@ import { configure } from "@testing-library/react"
import { resetAllWhenMocks } from "jest-when"
import * as matchers from "jest-extended"
import { mockRouter } from "ol-test-utilities/mocks/nextNavigation"
import preloadAll from "jest-next-dynamic-ts"

expect.extend(matchers)

@@ -85,6 +86,10 @@ jest.mock("next/navigation", () => {
}
})

beforeAll(async () => {
await preloadAll()
})

afterEach(() => {
/**
* Clear all mock call counts between tests.
69 changes: 47 additions & 22 deletions frontends/main/src/app-pages/HomePage/HomePage.tsx
Original file line number Diff line number Diff line change
@@ -1,16 +1,42 @@
"use client"

import React from "react"
import React, { Suspense } from "react"
import { Container, styled, theme } from "ol-components"
import HeroSearch from "@/page-components/HeroSearch/HeroSearch"
import BrowseTopicsSection from "./BrowseTopicsSection"
import NewsEventsSection from "./NewsEventsSection"
import TestimonialsSection from "./TestimonialsSection"
import ResourceCarousel from "@/page-components/ResourceCarousel/ResourceCarousel"
import PersonalizeSection from "./PersonalizeSection"
import * as carousels from "./carousels"
import dynamic from "next/dynamic"

const HeroSearch = dynamic(
() => import("@/page-components/HeroSearch/HeroSearch"),
{ ssr: true },
)

const TestimonialsSection = dynamic(() => import("./TestimonialsSection"), {
ssr: true,
})

const ResourceCarousel = dynamic(
() => import("@/page-components/ResourceCarousel/ResourceCarousel"),
{ ssr: true },
)

const PersonalizeSection = dynamic(() => import("./PersonalizeSection"), {
ssr: true,
})

const BrowseTopicsSection = dynamic(() => import("./BrowseTopicsSection"), {
ssr: true,
})

const NewsEventsSection = dynamic(() => import("./NewsEventsSection"), {
ssr: true,
})

const LearningResourceDrawer = dynamic(
() =>
import("@/page-components/LearningResourceDrawer/LearningResourceDrawer"),
{ ssr: false },
)

const FullWidthBackground = styled.div({
background: "linear-gradient(0deg, #FFF 0%, #E9ECEF 100%);",
paddingBottom: "80px",
@@ -44,11 +70,6 @@ const StyledContainer = styled(Container)({
},
})

const LearningResourceDrawer = dynamic(
() =>
import("@/page-components/LearningResourceDrawer/LearningResourceDrawer"),
)

const HomePage: React.FC<{ heroImageIndex: number }> = ({ heroImageIndex }) => {
return (
<>
@@ -57,21 +78,25 @@ const HomePage: React.FC<{ heroImageIndex: number }> = ({ heroImageIndex }) => {
<StyledContainer>
<HeroSearch imageIndex={heroImageIndex} />
<section>
<FeaturedCoursesCarousel
titleComponent="h2"
title="Featured Courses"
config={carousels.FEATURED_RESOURCES_CAROUSEL}
/>
<Suspense>
<FeaturedCoursesCarousel
titleComponent="h2"
title="Featured Courses"
config={carousels.FEATURED_RESOURCES_CAROUSEL}
/>
</Suspense>
</section>
</StyledContainer>
</FullWidthBackground>
<PersonalizeSection />
<Container component="section">
<MediaCarousel
titleComponent="h2"
title="Media"
config={carousels.MEDIA_CAROUSEL}
/>
<Suspense>
<MediaCarousel
titleComponent="h2"
title="Media"
config={carousels.MEDIA_CAROUSEL}
/>
</Suspense>
</Container>
<BrowseTopicsSection />
<TestimonialsSection />
17 changes: 9 additions & 8 deletions frontends/main/src/page-components/Header/Header.tsx
Original file line number Diff line number Diff line change
@@ -1,14 +1,9 @@
"use client"

import React, { FunctionComponent } from "react"
import dynamic from "next/dynamic"
import type { NavData } from "ol-components"
import {
styled,
AppBar,
NavDrawer,
Toolbar,
ActionButtonLink,
} from "ol-components"
import { styled, AppBar, Toolbar, ActionButtonLink } from "ol-components"
import {
RiSearch2Line,
RiPencilRulerLine,
@@ -25,7 +20,6 @@ import {
} from "@remixicon/react"
import { useToggle } from "ol-utilities"
import MITLogoLink from "@/components/MITLogoLink/MITLogoLink"
import UserMenu from "./UserMenu"
import { MenuButton } from "./MenuButton"
import {
DEPARTMENTS,
@@ -43,6 +37,13 @@ import {
} from "@/common/urls"
import { useUserMe } from "api/hooks/user"

const NavDrawer = dynamic(
() => import("ol-components").then((mod) => mod.NavDrawer),
{ ssr: false },
)

const UserMenu = dynamic(() => import("./UserMenu"), { ssr: false })

const Bar = styled(AppBar)(({ theme }) => ({
padding: "16px 8px",
backgroundColor: theme.custom.colors.navGray,
Original file line number Diff line number Diff line change
@@ -12,6 +12,7 @@ import {
} from "../Dialogs/AddToListDialog"
import { useResourceDrawerHref } from "../LearningResourceDrawer/useResourceDrawerHref"
import { useUserMe } from "api/hooks/user"
import { useMounted } from "api/ssr/useMounted"
import { LearningResource } from "api"
import { SignupPopover } from "../SignupPopover/SignupPopover"
import { useIsUserListMember } from "api/hooks/userLists"
@@ -100,6 +101,7 @@ const ResourceCard: React.FC<ResourceCardProps> = ({
inLearningPath,
onClick,
} = useResourceCard(resource)
const mounted = useMounted()
const CardComponent =
list && condensed
? LearningResourceListCardCondensed
@@ -112,8 +114,8 @@ const ResourceCard: React.FC<ResourceCardProps> = ({
onClick={onClick}
resource={resource}
href={resource ? getDrawerHref(resource.id) : undefined}
onAddToLearningPathClick={handleAddToLearningPathClick}
onAddToUserListClick={handleAddToUserListClick}
onAddToLearningPathClick={mounted ? handleAddToLearningPathClick : null}
onAddToUserListClick={mounted ? handleAddToUserListClick : null}
inUserList={inUserList}
inLearningPath={inLearningPath}
{...others}
1 change: 1 addition & 0 deletions frontends/package.json
Original file line number Diff line number Diff line change
@@ -63,6 +63,7 @@
"jest-environment-jsdom": "^29.5.0",
"jest-extended": "^4.0.2",
"jest-fail-on-console": "^3.3.1",
"jest-next-dynamic-ts": "^0.1.1",
"jest-watch-typeahead": "^2.2.2",
"jest-when": "^3.6.0",
"postcss-styled-syntax": "^0.7.0",
14 changes: 11 additions & 3 deletions yarn.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading