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

Jump to bottom

fix(app-render): make css and font respect assetPrefix #41455

Merged
merged 2 commits into from Oct 16, 2022
Merged

fix(app-render): make css and font respect assetPrefix #41455

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
91e81ce
fix(appDir): make css and font respect assetsPrefix
SukkaW Oct 16, 2022
64169e2
Merge branch 'canary' into link-assets-prefix
SukkaW Oct 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 10 additions & 8 deletions packages/next/server/app-render.tsx
View file
Open in desktop
Expand Up @@ -944,6 +944,8 @@ export async function renderToHTMLOrFlight(
ComponentMod.pages
)

const assetPrefix = renderOpts.assetPrefix || ''

/**
* Use the provided loader tree to create the React Component tree.
*/
Expand Down Expand Up @@ -1177,7 +1179,7 @@ export async function renderToHTMLOrFlight(
<link
key={fontFile}
rel="preload"
href={`/_next/${fontFile}`}
href={`${assetPrefix}/_next/${fontFile}`}
as="font"
type={`font/${ext}`}
crossOrigin="anonymous"
Comment on lines 1107 to 1113
Copy link
Contributor Author

@SukkaW SukkaW Oct 16, 2022 •
edited

Choose a reason for hiding this comment

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

Currently, we only load CSS and JS through the assetPrefix, so it doesn't require enabling CORS on the separate CDN. However, if we also load fonts through the CDN, it would be a breaking change as CORS is now required.

I added assetPrefix for the font here to match the existing implementation in pages/_document. But IMHO this should be reconsidered.

cc @hanneslund

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, good point!

Expand All @@ -1188,7 +1190,7 @@ export async function renderToHTMLOrFlight(
? stylesheets.map((href) => (
<link
rel="stylesheet"
href={`/_next/${href}${cacheBustingUrlSuffix}`}
href={`${assetPrefix}/_next/${href}${cacheBustingUrlSuffix}`}
// `Precedence` is an opt-in signal for React to handle
// resource loading and deduplication, etc:
// https://github.com/facebook/react/pull/25060
Expand Down Expand Up @@ -1423,7 +1425,7 @@ export async function renderToHTMLOrFlight(

return (
<AppRouter
assetPrefix={renderOpts.assetPrefix || ''}
assetPrefix={assetPrefix}
initialCanonicalUrl={initialCanonicalUrl}
initialTree={initialTree}
>
Expand Down Expand Up @@ -1469,7 +1471,7 @@ export async function renderToHTMLOrFlight(
polyfill.endsWith('.js') && !polyfill.endsWith('.module.js')
)
.map((polyfill) => ({
src: `${renderOpts.assetPrefix || ''}/_next/${polyfill}`,
src: `${assetPrefix}/_next/${polyfill}`,
integrity: subresourceIntegrityManifest?.[polyfill],
}))

Expand Down Expand Up @@ -1516,11 +1518,11 @@ export async function renderToHTMLOrFlight(
bootstrapScripts: [
...(subresourceIntegrityManifest
? buildManifest.rootMainFiles.map((src) => ({
src: `${renderOpts.assetPrefix || ''}/_next/` + src,
src: `${assetPrefix}/_next/` + src,
integrity: subresourceIntegrityManifest[src],
}))
: buildManifest.rootMainFiles.map(
(src) => `${renderOpts.assetPrefix || ''}/_next/` + src
(src) => `${assetPrefix}/_next/` + src
)),
],
},
Expand Down Expand Up @@ -1548,11 +1550,11 @@ export async function renderToHTMLOrFlight(
// Include hydration scripts in the HTML
bootstrapScripts: subresourceIntegrityManifest
? buildManifest.rootMainFiles.map((src) => ({
src: `${renderOpts.assetPrefix || ''}/_next/` + src,
src: `${assetPrefix}/_next/` + src,
integrity: subresourceIntegrityManifest[src],
}))
: buildManifest.rootMainFiles.map(
(src) => `${renderOpts.assetPrefix || ''}/_next/` + src
(src) => `${assetPrefix}/_next/` + src
),
},
})
Expand Down