From 4f5f4769e5c27fd409e9df707edb88ab4b6f243b Mon Sep 17 00:00:00 2001 From: Josh Story Date: Fri, 28 Apr 2023 15:50:20 -0700 Subject: [PATCH] preload fonts using ReactDOM.preload (#48931) This PR updates the way we preload fonts. Previously we tracked which fonts we needed to preload for each layer and rendered a `` tag for each preloadable font. This unfortunately gets blocked by data fetching and we want to be able to hint these preloads as soon as possible. Now that React support Float methods in RSC we can use `ReactDOM.preload(..., { as: "font" })` to implement this functionality This PR makes the following changes 1. expose a `preloadFont` method through the RSC graph 2. expose a `preconnect` metho through the RSC graph 3. refactor the preloads generation to use `preloadFont` instead of rendering a preload link 4. If there are no fonts to preload but fonts are being used in CSS then a `preconnect` asset origin is called instead of rendering a preconnect link 5. instead of emitting a data attribute per font preload indicating whether the project is using size-adjust we now emit a single global meta tag. In the future we may get more granular about which fonts are being size adjusted. In the meantime the current hueristic is to add `-s` to the filename so it can still be inferred. In the process of completing this work I discovered there were some bugs in how the preconnect logic was originally implemented. Previously it was possible to get multiple preconnects per render. Additionally the preconnect href was always `"/"` which is not correct if you are hosting your fonts at a CDN. The refactor fixed both of these issues I want to do a larger refactor of the asset loading logic in App-Render but I'll save that for a couple weeks from now Additionally, the serialized output of preloads now omits the word anonymous when using crossorigin so tests were updated to reflect `crossorigin=""` Additionally, tests were updated to no longer look for the size-adjust data attribute on preloads Additionally, There is a note about leaving a `{null}` render in place to avoid a conflict with how the router models lazy trees. I'll follow up with a PR addressing this Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> --- .../js/src/entry/app/layout-entry.tsx | 6 +- .../build/webpack/loaders/next-app-loader.ts | 2 +- .../next/src/server/app-render/app-render.tsx | 92 +++++++++---------- .../app-render/get-css-inlined-link-tags.tsx | 2 +- ...ink-tags.tsx => get-preloadable-fonts.tsx} | 22 ++--- .../src/server/app-render/rsc/preloads.ts | 12 +++ test/e2e/app-dir/next-font/next-font.test.ts | 33 +++---- 7 files changed, 84 insertions(+), 85 deletions(-) rename packages/next/src/server/app-render/{get-preloaded-font-files-inline-link-tags.tsx => get-preloadable-fonts.tsx} (68%) diff --git a/packages/next-swc/crates/next-core/js/src/entry/app/layout-entry.tsx b/packages/next-swc/crates/next-core/js/src/entry/app/layout-entry.tsx index 3aab98b573aab..f9b0a9dd3c877 100644 --- a/packages/next-swc/crates/next-core/js/src/entry/app/layout-entry.tsx +++ b/packages/next-swc/crates/next-core/js/src/entry/app/layout-entry.tsx @@ -18,4 +18,8 @@ export { renderToReadableStream, decodeReply, } from 'next/dist/compiled/react-server-dom-webpack/server.edge' -export { preloadStyle } from 'next/dist/server/app-render/rsc/preloads' +export { + preloadStyle, + preloadFont, + preconnect, +} from 'next/dist/server/app-render/rsc/preloads' diff --git a/packages/next/src/build/webpack/loaders/next-app-loader.ts b/packages/next/src/build/webpack/loaders/next-app-loader.ts index 7d7eddaa8c7b5..a86c6a8cbf2ad 100644 --- a/packages/next/src/build/webpack/loaders/next-app-loader.ts +++ b/packages/next/src/build/webpack/loaders/next-app-loader.ts @@ -594,7 +594,7 @@ const nextAppLoader: AppLoader = async function nextAppLoader() { export { renderToReadableStream, decodeReply } from 'next/dist/compiled/react-server-dom-webpack/server.edge' export const __next_app_webpack_require__ = __webpack_require__ - export { preloadStyle } from 'next/dist/server/app-render/rsc/preloads' + export { preloadStyle, preloadFont, preconnect } from 'next/dist/server/app-render/rsc/preloads' ` return result diff --git a/packages/next/src/server/app-render/app-render.tsx b/packages/next/src/server/app-render/app-render.tsx index 88e54fa36d27b..92dfa6cb1c46e 100644 --- a/packages/next/src/server/app-render/app-render.tsx +++ b/packages/next/src/server/app-render/app-render.tsx @@ -9,6 +9,7 @@ import type { RenderOpts, Segment, } from './types' + import type { StaticGenerationAsyncStorage } from '../../client/components/static-generation-async-storage' import type { StaticGenerationBailout } from '../../client/components/static-generation-bailout' import type { RequestAsyncStorage } from '../../client/components/request-async-storage' @@ -60,7 +61,7 @@ import { getShortDynamicParamType } from './get-short-dynamic-param-type' import { getSegmentParam } from './get-segment-param' import { getCssInlinedLinkTags } from './get-css-inlined-link-tags' import { getServerCSSForEntries } from './get-server-css-for-entries' -import { getPreloadedFontFilesInlineLinkTags } from './get-preloaded-font-files-inline-link-tags' +import { getPreloadableFonts } from './get-preloadable-fonts' import { getScriptNonceFromHeader } from './get-script-nonce-from-header' import { renderToString } from './render-to-string' import { parseAndValidateFlightRouterState } from './parse-and-validate-flight-router-state' @@ -155,6 +156,8 @@ export async function renderToHTMLOrFlight( nextConfigOutput, } = renderOpts + const appUsingSizeAdjust = nextFontManifest?.appUsingSizeAdjust + const clientReferenceManifest = renderOpts.clientReferenceManifest! const serverCSSManifest = renderOpts.serverCSSManifest! @@ -406,10 +409,7 @@ export async function renderToHTMLOrFlight( layoutOrPagePath: string | undefined injectedCSS: Set injectedFontPreloadTags: Set - }): { - styles: React.ReactNode - preloads: React.ReactNode - } => { + }): React.ReactNode => { const stylesheets: string[] = layoutOrPagePath ? getCssInlinedLinkTags( clientReferenceManifest, @@ -422,49 +422,35 @@ export async function renderToHTMLOrFlight( : [] const preloadedFontFiles = layoutOrPagePath - ? getPreloadedFontFilesInlineLinkTags( + ? getPreloadableFonts( serverCSSManifest!, nextFontManifest, serverCSSForEntries, layoutOrPagePath, injectedFontPreloadTagsWithCurrentLayout ) - : [] + : null - const preloads = ( - <> - {preloadedFontFiles?.length === 0 ? ( - <> - - - ) : null} - {preloadedFontFiles - ? preloadedFontFiles.map((fontFile) => { - const ext = /\.(woff|woff2|eot|ttf|otf)$/.exec(fontFile)![1] - return ( - - ) - }) - : null} - - ) + if (preloadedFontFiles) { + if (preloadedFontFiles.length) { + for (let i = 0; i < preloadedFontFiles.length; i++) { + const fontFilename = preloadedFontFiles[i] + const ext = /\.(woff|woff2|eot|ttf|otf)$/.exec(fontFilename)![1] + const type = `font/${ext}` + const href = `${assetPrefix}/_next/${fontFilename}` + ComponentMod.preloadFont(href, type) + } + } else { + try { + let url = new URL(assetPrefix) + ComponentMod.preconnect(url.origin, 'anonymous') + } catch (error) { + // assetPrefix must not be a fully qualified domain name. We assume + // we should preconnect to same origin instead + ComponentMod.preconnect('/', 'anonymous') + } + } + } const styles = stylesheets ? stylesheets.map((href, index) => { @@ -493,10 +479,7 @@ export async function renderToHTMLOrFlight( }) : null - return { - styles, - preloads, - } + return styles } const parseLoaderTree = (tree: LoaderTree) => { @@ -560,7 +543,7 @@ export async function renderToHTMLOrFlight( injectedFontPreloadTags ) - const { styles, preloads } = getLayerAssets({ + const styles = getLayerAssets({ layoutOrPagePath, injectedCSS: injectedCSSWithCurrentLayout, injectedFontPreloadTags: injectedFontPreloadTagsWithCurrentLayout, @@ -911,7 +894,15 @@ export async function renderToHTMLOrFlight( ) : ( )} - {preloads} + {/* This null is currently critical. The wrapped Component can render null and if there was not fragment + surrounding it this would look like a pending tree data state on the client which will cause an errror + and break the app. Long-term we need to move away from using null as a partial tree identifier since it + is a valid return type for the components we wrap. Once we make this change we can safely remove the + fragment. The reason the extra null here is required is that fragments which only have 1 child are elided. + If the Component above renders null the actual treedata will look like `[null, null]`. If we remove the extra + null it will look like `null` (the array is elided) and this is what confuses the client router. + TODO-APP update router to use a Symbol for partial tree detection */} + {null} ) }, @@ -1040,7 +1031,7 @@ export async function renderToHTMLOrFlight( const { layoutOrPagePath } = parseLoaderTree(loaderTreeToFilter) - const { styles } = getLayerAssets({ + const styles = getLayerAssets({ layoutOrPagePath, injectedCSS: new Set(injectedCSS), injectedFontPreloadTags: new Set(injectedFontPreloadTags), @@ -1074,7 +1065,7 @@ export async function renderToHTMLOrFlight( injectedCSSWithCurrentLayout, true ) - getPreloadedFontFilesInlineLinkTags( + getPreloadableFonts( serverCSSManifest!, nextFontManifest, serverCSSForEntries, @@ -1155,6 +1146,7 @@ export async function renderToHTMLOrFlight( searchParams={providedSearchParams} getDynamicParamFromSegment={getDynamicParamFromSegment} /> + {appUsingSizeAdjust ? : null} ), injectedCSS: new Set(), @@ -1292,6 +1284,7 @@ export async function renderToHTMLOrFlight( searchParams={providedSearchParams} getDynamicParamFromSegment={getDynamicParamFromSegment} /> + {appUsingSizeAdjust ? : null} } globalErrorComponent={GlobalError} @@ -1499,6 +1492,7 @@ export async function renderToHTMLOrFlight( searchParams={providedSearchParams} getDynamicParamFromSegment={getDynamicParamFromSegment} /> + {appUsingSizeAdjust ? : null} diff --git a/packages/next/src/server/app-render/get-css-inlined-link-tags.tsx b/packages/next/src/server/app-render/get-css-inlined-link-tags.tsx index df8e726d9be3d..47fd9301ddf79 100644 --- a/packages/next/src/server/app-render/get-css-inlined-link-tags.tsx +++ b/packages/next/src/server/app-render/get-css-inlined-link-tags.tsx @@ -5,7 +5,7 @@ import { import { getClientReferenceModuleKey } from '../../lib/client-reference' /** - * Get inline tags based on server CSS manifest. Only used when rendering to HTML. + * Get external stylesheet link hrefs based on server CSS manifest. */ export function getCssInlinedLinkTags( clientReferenceManifest: ClientReferenceManifest, diff --git a/packages/next/src/server/app-render/get-preloaded-font-files-inline-link-tags.tsx b/packages/next/src/server/app-render/get-preloadable-fonts.tsx similarity index 68% rename from packages/next/src/server/app-render/get-preloaded-font-files-inline-link-tags.tsx rename to packages/next/src/server/app-render/get-preloadable-fonts.tsx index 6559cf58961ea..9fcef03af032f 100644 --- a/packages/next/src/server/app-render/get-preloaded-font-files-inline-link-tags.tsx +++ b/packages/next/src/server/app-render/get-preloadable-fonts.tsx @@ -2,9 +2,13 @@ import { NextFontManifest } from '../../build/webpack/plugins/next-font-manifest import { ClientCSSReferenceManifest } from '../../build/webpack/plugins/flight-manifest-plugin' /** - * Get inline tags based on server CSS manifest and next/font manifest. Only used when rendering to HTML. + * Get hrefs for fonts to preload + * Returns null if there are no fonts at all. + * Returns string[] if there are fonts to preload (font paths) + * Returns empty string[] if there are fonts but none to preload and no other fonts have been preloaded + * Returns null if there are fonts but none to preload and at least some were previously preloaded */ -export function getPreloadedFontFilesInlineLinkTags( +export function getPreloadableFonts( serverCSSManifest: ClientCSSReferenceManifest, nextFontManifest: NextFontManifest | undefined, serverCSSForEntries: string[], @@ -39,15 +43,11 @@ export function getPreloadedFontFilesInlineLinkTags( } } - // If we find an entry in the manifest but it's empty, add a preconnect tag by returning null. - // Only render a preconnect tag if we previously didn't preload any fonts. - if ( - !foundFontUsage || - (fontFiles.size === 0 && injectedFontPreloadTags.size > 0) - ) { + if (fontFiles.size) { + return [...fontFiles].sort() + } else if (foundFontUsage && injectedFontPreloadTags.size === 0) { + return [] + } else { return null } - - // Sorting to make order deterministic - return [...fontFiles].sort() } diff --git a/packages/next/src/server/app-render/rsc/preloads.ts b/packages/next/src/server/app-render/rsc/preloads.ts index 13ccaa2f820a9..85ff5abbc33bd 100644 --- a/packages/next/src/server/app-render/rsc/preloads.ts +++ b/packages/next/src/server/app-render/rsc/preloads.ts @@ -10,3 +10,15 @@ const stylePreloadOptions = { as: 'style' } export function preloadStyle(href: string) { ;(ReactDOM as any).preload(href, stylePreloadOptions) } + +export function preloadFont(href: string, type: string) { + ;(ReactDOM as any).preload(href, { as: 'font', type }) +} + +export function preconnect(href: string, crossOrigin?: string) { + if (typeof crossOrigin === 'string') { + ;(ReactDOM as any).preconnect(href, { crossOrigin }) + } else { + ;(ReactDOM as any).preconnect(href) + } +} diff --git a/test/e2e/app-dir/next-font/next-font.test.ts b/test/e2e/app-dir/next-font/next-font.test.ts index c1859b1e1ca92..b80e98f08b741 100644 --- a/test/e2e/app-dir/next-font/next-font.test.ts +++ b/test/e2e/app-dir/next-font/next-font.test.ts @@ -235,27 +235,24 @@ describe.each([['app'], ['app-old']])('%s', (fixture: string) => { expect(getAttrs($('link[as="font"]'))).toEqual([ { as: 'font', - crossorigin: 'anonymous', + crossorigin: '', href: '/_next/static/media/b2104791981359ae-s.p.woff2', rel: 'preload', type: 'font/woff2', - 'data-next-font': 'size-adjust', }, { as: 'font', - crossorigin: 'anonymous', + crossorigin: '', href: '/_next/static/media/b61859a50be14c53-s.p.woff2', rel: 'preload', type: 'font/woff2', - 'data-next-font': 'size-adjust', }, { as: 'font', - crossorigin: 'anonymous', + crossorigin: '', href: '/_next/static/media/e9b9dc0d8ba35f48-s.p.woff2', rel: 'preload', type: 'font/woff2', - 'data-next-font': 'size-adjust', }, ]) }) @@ -270,27 +267,24 @@ describe.each([['app'], ['app-old']])('%s', (fixture: string) => { expect(getAttrs($('link[as="font"]'))).toEqual([ { as: 'font', - crossorigin: 'anonymous', + crossorigin: '', href: '/_next/static/media/e1053f04babc7571-s.p.woff2', rel: 'preload', type: 'font/woff2', - 'data-next-font': 'size-adjust', }, { as: 'font', - crossorigin: 'anonymous', + crossorigin: '', href: '/_next/static/media/e9b9dc0d8ba35f48-s.p.woff2', rel: 'preload', type: 'font/woff2', - 'data-next-font': 'size-adjust', }, { as: 'font', - crossorigin: 'anonymous', + crossorigin: '', href: '/_next/static/media/feab2c68f2a8e9a4-s.p.woff2', rel: 'preload', type: 'font/woff2', - 'data-next-font': 'size-adjust', }, ]) }) @@ -305,19 +299,17 @@ describe.each([['app'], ['app-old']])('%s', (fixture: string) => { expect(getAttrs($('link[as="font"]'))).toEqual([ { as: 'font', - crossorigin: 'anonymous', + crossorigin: '', href: '/_next/static/media/75c5faeeb9c86969-s.p.woff2', rel: 'preload', type: 'font/woff2', - 'data-next-font': 'size-adjust', }, { as: 'font', - crossorigin: 'anonymous', + crossorigin: '', href: '/_next/static/media/e9b9dc0d8ba35f48-s.p.woff2', rel: 'preload', type: 'font/woff2', - 'data-next-font': 'size-adjust', }, ]) }) @@ -332,19 +324,17 @@ describe.each([['app'], ['app-old']])('%s', (fixture: string) => { expect(getAttrs($('link[as="font"]'))).toEqual([ { as: 'font', - crossorigin: 'anonymous', + crossorigin: '', href: '/_next/static/media/568e4c6d8123c4d6-s.p.woff2', rel: 'preload', type: 'font/woff2', - 'data-next-font': 'size-adjust', }, { as: 'font', - crossorigin: 'anonymous', + crossorigin: '', href: '/_next/static/media/e9b9dc0d8ba35f48-s.p.woff2', rel: 'preload', type: 'font/woff2', - 'data-next-font': 'size-adjust', }, ]) }) @@ -359,10 +349,9 @@ describe.each([['app'], ['app-old']])('%s', (fixture: string) => { // Preconnect expect($('link[rel="preconnect"]').length).toBe(1) expect($('link[rel="preconnect"]').get(0).attribs).toEqual({ - crossorigin: 'anonymous', + crossorigin: '', href: '/', rel: 'preconnect', - 'data-next-font': 'size-adjust', }) // Preload expect($('link[as="font"]').length).toBe(0)