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.

Sign up for GitHub

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

chore(perf): Avoid conflicting hints to preload and defer initial JS #14992

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+0 −8
Open

chore(perf): Avoid conflicting hints to preload and defer initial JS #14992

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e0fbdae
wip(perf): test removing defer hint from initial scripts
joeyAghion Dec 12, 2024
13cf683
wip(perf): test fully deferring initial js bundles
joeyAghion Dec 13, 2024
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
4 changes: 0 additions & 4 deletions src/Server/middleware/linkHeadersMiddleware.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import type { NextFunction } from "express"
import type { ArtsyRequest, ArtsyResponse } from "./artsyExpress"
import { CDN_URL, GEMINI_CLOUDFRONT_URL, WEBFONT_URL } from "Server/config"
import { getWebpackEarlyHints } from "Server/getWebpackEarlyHints"

/**
* Link headers allow 103: Early Hints to be sent to the client (by Cloudflare).
Expand All @@ -13,8 +12,6 @@ export function linkHeadersMiddleware(
res: ArtsyResponse,
next: NextFunction
) {
const { linkHeaders } = getWebpackEarlyHints()

if (!res.headersSent) {
res.header("Link", [
`<${CDN_URL}>; rel=preconnect; crossorigin`,
Expand All @@ -24,7 +21,6 @@ export function linkHeadersMiddleware(
`<${WEBFONT_URL}/ll-unica77_regular.woff2>; rel=preload; as=font; crossorigin`,
`<${WEBFONT_URL}/ll-unica77_medium.woff2>; rel=preload; as=font; crossorigin`,
`<${WEBFONT_URL}/ll-unica77_italic.woff2>; rel=preload; as=font; crossorigin`,
...linkHeaders,
Copy link
Member

Choose a reason for hiding this comment

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

We're addressing the warnings, but do you remember that wild decrease in stats that we saw from calibre? It was due to adding this.

Are we sure here?

Copy link
Member

Choose a reason for hiding this comment

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

Things have been crazy today and i haven't been following closely, but are you sure that removing defer in collectAssets doesn't address all the things in the best way?

We're loading so much js. We want to hint that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, no I'm not sure. My tests were pretty clear that the js should stay deferred ("lab score" went from 70 to 55 otherwise). This change eliminated the warnings, but didn't score any better.

If it were the middle of the week, I'd say let's try it for an official Calibre run or 2. Since it's not, let's wait and revisit next week. I think the more impactful change will be to preload the main image (#14974), ahead of these assets.

Copy link
Member

@damassi damassi Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We can run a couple manual snapshots if that helps anything, but i see what you mean in terms of getting the weeks average. Either way!

])
}

Expand Down
4 changes: 0 additions & 4 deletions src/System/Router/renderServerApp.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import { loadAssetManifest } from "Server/manifest"
import { ENABLE_SSR_STREAMING } from "Server/config"
import { getENV } from "Utils/getENV"
import { ServerAppResults } from "System/Router/serverRouter"
import { getWebpackEarlyHints } from "Server/getWebpackEarlyHints"
import { RenderToStreamResult } from "System/Router/Utils/renderToStream"
import { buildHtmlTemplate, HTMLProps } from "html"

Expand Down Expand Up @@ -48,15 +47,12 @@ export const renderServerApp = ({

const scripts = extractScriptTags?.()

const { linkPreloadTags } = getWebpackEarlyHints()

const options: HTMLProps = {
cdnUrl: NODE_ENV === "production" ? (CDN_URL as string) : "",
content: {
body: html,
sharifyData: sharify.script(),
head: headTagsString,
linkPreloadTags,
scripts,
style: styleTags,
},
Expand Down