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

Disable About page prefetching #1938

Merged
merged 4 commits into from
Jan 7, 2025
Merged

Conversation

jonkafton
Copy link
Contributor

@jonkafton jonkafton commented Jan 3, 2025

What are the relevant tickets?

Relates to https://github.com/mitodl/hq/issues/6290

Description (What does it do?)

A link to the About page features prominently on the Homepage. As a result, Next.js gives priority to prefetching the page (by default, next/link loads pages in anticipation of the user navigating to them). Scripts for the About page are contributing to the Total Blocking Time during the initial Homepage load phase. Fast navigation to the About page is good to have, but not at the expense of initial load performance of other pages.

This change disabled prefetching behavior on links to the About page. This also moves the hash fragment string as AboutPage.tsx was still showing up in the call tree while profiling the Homepage (unexpectedly for a named export). See "Prevent the About page from prefetching" in https://github.com/mitodl/hq/issues/6290#issuecomment-2568379941.

Also set the "Non-Degree Learning" to scroll top on navigate as noticed that wasn't working correctly.

How can this be tested?

  • Check that no calls are made for an RSC payload for the About page while loading the Homepage (these look like /about?_rsc=1wtp7).
  • Perhaps run a load performance profile in the Chrome Dev Tools performance tab and search the call tree for AboutPage.tsx to ensure it's not there (running Next.js in dev mode only as files are not source mapped here not true, plus prefetching only happens in production).
  • Check that the About page loads scrolled to top when using the "Non-Degree Learning" link in the Hero search.

Additional Context

@ChristopherChudzicki ChristopherChudzicki self-assigned this Jan 6, 2025
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.

Minor request in regards to scroll.

Suggestion: I see prefetching also happens for several other pages, like topics, search, etc. Also individual channel pages.

I sort of think we should globally disable prefetch. I don't see an easy way to do this. Next best option is a global Link component wrapping NExtJS's link, maybe

const Link = ({ prefetch = false, href, ...others }) => {
  const router = useRouter()
  const doPrefetch = () => router.pretch(href)
  return <NextLink
    prefetch={prefetch}
    href={href}
    onFocus={doPrefetch}
    onHover={doPrefetch}
    {...others}
  />
}

I believe that older versions of NextJS prefetched on hover even when prefetch={false}, but the current version seems to disable it fully when false. See alse:

**Note: ** This could be done separately, and I haven't investigated the flamegraph for topics/search page, but I can see the code runs by inserting some console.logs into the files.

href={`${ABOUT}#${NON_DEGREE_LEARNING_FRAGMENT_IDENTIFIER}`}
href={`${ABOUT}#${ABOUT_NON_DEGREE_LEARNING_FRAGMENT}`}
prefetch={false}
scroll={false}
Copy link
Contributor

Choose a reason for hiding this comment

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

Also set the "Non-Degree Learning" to scroll top on navigate as noticed that wasn't working correctly.

In what sense was it not working correctly? Currently works as I expect on https://rc.learn.mit.edu/.

For me, scrolls to here:

Screenshot 2025-01-06 at 1 47 58 PM

It doesn't scroll exactly to the header, but I think that's normal.

Request: Leave scroll behavior as-was.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looked off to me because the page isn't long enough for the hash fragment to load scrolled to top. I agree though that that doesn't actually constitute not working correctly. I've changed it back.

Copy link
Contributor

Choose a reason for hiding this comment

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

ahhh, I didn't think about it enough to explain why it doesn't go all the way to the top.

That makes sense, though.

@@ -26,7 +27,6 @@ import {
RiTimeLine,
RiVerifiedBadgeLine,
} from "@remixicon/react"
import { NON_DEGREE_LEARNING_FRAGMENT_IDENTIFIER } from "@/app-pages/AboutPage/AboutPage"
Copy link
Contributor

Choose a reason for hiding this comment

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

btw: This import goes against our hierearchy rules, and should have been forbidden.

Our lint rule enforcing this seems to be broken for two reasons:

  • pages needs to be renamed app-pages
  • "*/tsconfig.json" should be `"**/tsconfig.json"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good shout - I've updated the rules. This required moving the carousels config into common. This does import a type from @/page-components/ResourceCarousel/ResourceCarousel. Perhaps we should disallow or allow only for types.

@jonkafton
Copy link
Contributor Author

Suggestion: I see prefetching also happens for several other pages, like topics, search, etc. Also individual channel pages.

I sort of think we should globally disable prefetch. I don't see an easy way to do this. Next best option is a global Link component wrapping NExtJS's link, maybe

The prefetching is marginally useful and nice to have so long as it doesn't impact performance. The about page prefetch was disproportionately showing up as thread blocking - Next.js detects links in the viewport to prioritize the prefetch, though that doesn't explain why topics and search don't equally show up in the load call tree, unless page prominence additionally factors in. The main argument for me for disabling everywhere would be that performance impacts are hard to spot and could easily make their way back in unnoticed.

@jonkafton jonkafton merged commit 7f56a03 into main Jan 7, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants