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

Making font optimization as default #19758

Merged
merged 18 commits into from
Dec 4, 2020
Merged
Show file tree
Hide file tree
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
7 changes: 1 addition & 6 deletions packages/next/build/webpack-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -970,9 +970,6 @@ export default async function getBaseWebpackConfig(
'process.env.__NEXT_REACT_MODE': JSON.stringify(
config.experimental.reactMode
),
'process.env.__NEXT_OPTIMIZE_FONTS': JSON.stringify(
config.experimental.optimizeFonts && !dev
),
'process.env.__NEXT_OPTIMIZE_IMAGES': JSON.stringify(
config.experimental.optimizeImages
),
Expand Down Expand Up @@ -1077,8 +1074,7 @@ export default async function getBaseWebpackConfig(
new ProfilingPlugin({
tracer,
}),
config.experimental.optimizeFonts &&
!dev &&
!dev &&
isServer &&
(function () {
const {
Expand Down Expand Up @@ -1166,7 +1162,6 @@ export default async function getBaseWebpackConfig(
plugins: config.experimental.plugins,
reactStrictMode: config.reactStrictMode,
reactMode: config.experimental.reactMode,
optimizeFonts: config.experimental.optimizeFonts,
optimizeImages: config.experimental.optimizeImages,
scrollRestoration: config.experimental.scrollRestoration,
basePath: config.basePath,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -370,15 +370,12 @@ export function getPageHandler(ctx: ServerlessHandlerCtx) {

const previewData = tryGetPreviewData(req, res, options.previewProps)
const isPreviewMode = previewData !== false
if (process.env.__NEXT_OPTIMIZE_FONTS) {
renderOpts.optimizeFonts = true
/**
* __webpack_require__.__NEXT_FONT_MANIFEST__ is added by
* font-stylesheet-gathering-plugin
*/
// @ts-ignore
renderOpts.fontManifest = __webpack_require__.__NEXT_FONT_MANIFEST__
}
/**
* __webpack_require__.__NEXT_FONT_MANIFEST__ is added by
* font-stylesheet-gathering-plugin
*/
// @ts-ignore
renderOpts.fontManifest = __webpack_require__.__NEXT_FONT_MANIFEST__
let result = await renderToHTML(
req,
res,
Expand Down
1 change: 0 additions & 1 deletion packages/next/export/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -484,7 +484,6 @@ Read more: https://err.sh/next.js/export-image-api`
subFolders,
buildExport: options.buildExport,
serverless: isTargetLikeServerless(nextConfig.target),
optimizeFonts: nextConfig.experimental.optimizeFonts,
optimizeImages: nextConfig.experimental.optimizeImages,
optimizeCss: nextConfig.experimental.optimizeCss,
})
Expand Down
23 changes: 2 additions & 21 deletions packages/next/export/worker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ interface ExportPageInput {
serverRuntimeConfig: string
subFolders: string
serverless: boolean
optimizeFonts: boolean
optimizeImages: boolean
optimizeCss: any
}
Expand All @@ -67,7 +66,6 @@ interface RenderOpts {
ampSkipValidation?: boolean
hybridAmp?: boolean
inAmpMode?: boolean
optimizeFonts?: boolean
optimizeImages?: boolean
optimizeCss?: any
fontManifest?: FontManifest
Expand All @@ -92,7 +90,6 @@ export default async function exportPage({
serverRuntimeConfig,
subFolders,
serverless,
optimizeFonts,
optimizeImages,
optimizeCss,
}: ExportPageInput): Promise<ExportPageResults> {
Expand Down Expand Up @@ -250,14 +247,10 @@ export default async function exportPage({
{
ampPath: renderAmpPath,
/// @ts-ignore
optimizeFonts,
/// @ts-ignore
optimizeImages,
/// @ts-ignore
optimizeCss,
fontManifest: optimizeFonts
? requireFontManifest(distDir, serverless)
: null,
fontManifest: requireFontManifest(distDir, serverless),
locale: locale!,
locales: renderOpts.locales!,
},
Expand Down Expand Up @@ -295,15 +288,6 @@ export default async function exportPage({
html = components.Component
queryWithAutoExportWarn()
} else {
/**
* This sets environment variable to be used at the time of static export by head.tsx.
* Using this from process.env allows targeting both serverless and SSR by calling
* `process.env.__NEXT_OPTIMIZE_FONTS`.
* TODO(prateekbh@): Remove this when experimental.optimizeFonts are being cleaned up.
*/
if (optimizeFonts) {
process.env.__NEXT_OPTIMIZE_FONTS = JSON.stringify(true)
}
if (optimizeImages) {
process.env.__NEXT_OPTIMIZE_IMAGES = JSON.stringify(true)
}
Expand All @@ -315,12 +299,9 @@ export default async function exportPage({
...renderOpts,
ampPath: renderAmpPath,
params,
optimizeFonts,
optimizeImages,
optimizeCss,
fontManifest: optimizeFonts
? requireFontManifest(distDir, serverless)
: null,
fontManifest: requireFontManifest(distDir, serverless),
locale: locale as string,
}
// @ts-ignore
Expand Down
2 changes: 1 addition & 1 deletion packages/next/next-server/lib/head.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ function reduceComponents(
.reverse()
.map((c: React.ReactElement<any>, i: number) => {
const key = c.key || i
if (process.env.__NEXT_OPTIMIZE_FONTS && !props.inAmpMode) {
if (process.env.NODE_ENV !== 'development' && !props.inAmpMode) {
if (
c.type === 'link' &&
c.props['href'] &&
Expand Down
27 changes: 15 additions & 12 deletions packages/next/next-server/lib/post-process.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ const MAXIMUM_IMAGE_PRELOADS = 2
const IMAGE_PRELOAD_SIZE_THRESHOLD = 2500

type postProcessOptions = {
optimizeFonts: boolean
optimizeImages: boolean
}

Expand Down Expand Up @@ -143,10 +142,20 @@ class FontOptimizerMiddleware implements PostProcessMiddleware {
continue
}
const fontContent = options.getFontDefinition(url)
result = result.replace(
'</head>',
`<style data-href="${url}">${fontContent}</style></head>`
)
if (!fontContent) {
/**
* In case of unreachable font definitions, fallback to default link tag.
*/
result = result.replace(
'</head>',
`<link rel="stylesheet" href="${url}"/></head>`
)
} else {
result = result.replace(
'</head>',
`<style data-href="${url}">${fontContent}</style></head>`
)
}
}
return result
}
Expand Down Expand Up @@ -251,13 +260,7 @@ function sourceIsSupportedType(imgSrc: string): boolean {
}

// Initialization
registerPostProcessor(
'Inline-Fonts',
new FontOptimizerMiddleware(),
// Using process.env because passing Experimental flag through loader is not possible.
// @ts-ignore
(options) => options.optimizeFonts || process.env.__NEXT_OPTIMIZE_FONTS
)
registerPostProcessor('Inline-Fonts', new FontOptimizerMiddleware(), () => true)

registerPostProcessor(
'Preload Images',
Expand Down
1 change: 0 additions & 1 deletion packages/next/next-server/server/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ const defaultConfig: { [key: string]: any } = {
workerThreads: false,
pageEnv: false,
productionBrowserSourceMaps: false,
optimizeFonts: false,
optimizeImages: false,
optimizeCss: false,
scrollRestoration: false,
Expand Down
16 changes: 5 additions & 11 deletions packages/next/next-server/server/next-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,6 @@ export default class Server {
customServer?: boolean
ampOptimizerConfig?: { [key: string]: any }
basePath: string
optimizeFonts: boolean
images: string
fontManifest: FontManifest
optimizeImages: boolean
Expand Down Expand Up @@ -202,11 +201,9 @@ export default class Server {
ampOptimizerConfig: this.nextConfig.experimental.amp?.optimizer,
basePath: this.nextConfig.basePath,
images: JSON.stringify(this.nextConfig.images),
optimizeFonts: this.nextConfig.experimental.optimizeFonts && !dev,
fontManifest:
this.nextConfig.experimental.optimizeFonts && !dev
? requireFontManifest(this.distDir, this._isLikeServerless)
: null,
fontManifest: !dev
? requireFontManifest(this.distDir, this._isLikeServerless)
: null,
optimizeImages: this.nextConfig.experimental.optimizeImages,
optimizeCss: this.nextConfig.experimental.optimizeCss,
}
Expand Down Expand Up @@ -268,12 +265,9 @@ export default class Server {
/**
* This sets environment variable to be used at the time of SSR by head.tsx.
* Using this from process.env allows targetting both serverless and SSR by calling
* `process.env.__NEXT_OPTIMIZE_FONTS`.
* TODO(prateekbh@): Remove this when experimental.optimizeFonts are being clened up.
* `process.env.__NEXT_OPTIMIZE_IMAGES`.
* TODO(atcastle@): Remove this when experimental.optimizeImages are being clened up.
*/
if (this.renderOpts.optimizeFonts) {
process.env.__NEXT_OPTIMIZE_FONTS = JSON.stringify(true)
}
if (this.renderOpts.optimizeImages) {
process.env.__NEXT_OPTIMIZE_IMAGES = JSON.stringify(true)
}
Expand Down
22 changes: 9 additions & 13 deletions packages/next/next-server/server/render.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,6 @@ export type RenderOptsPartial = {
previewProps: __ApiPreviewProps
basePath: string
unstable_runtimeJS?: false
optimizeFonts: boolean
fontManifest?: FontManifest
optimizeImages: boolean
optimizeCss: any
Expand Down Expand Up @@ -1051,18 +1050,15 @@ export async function renderToHTML(
}

// Avoid postProcess if both flags are false
if (process.env.__NEXT_OPTIMIZE_FONTS || process.env.__NEXT_OPTIMIZE_IMAGES) {
html = await postProcess(
html,
{
getFontDefinition,
},
{
optimizeFonts: renderOpts.optimizeFonts,
optimizeImages: renderOpts.optimizeImages,
}
)
}
html = await postProcess(
html,
{
getFontDefinition,
},
{
optimizeImages: renderOpts.optimizeImages,
}
)

if (renderOpts.optimizeCss) {
// eslint-disable-next-line import/no-extraneous-dependencies
Expand Down
4 changes: 2 additions & 2 deletions packages/next/pages/_document.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ export class Head extends Component<
)
})

if (process.env.__NEXT_OPTIMIZE_FONTS) {
if (process.env.NODE_ENV !== 'development') {
cssLinkElements = this.makeStylesheetInert(
cssLinkElements
) as ReactElement[]
Expand Down Expand Up @@ -369,7 +369,7 @@ export class Head extends Component<
)
}

if (process.env.__NEXT_OPTIMIZE_FONTS && !inAmpMode) {
if (process.env.NODE_ENV !== 'development' && !inAmpMode) {
children = this.makeStylesheetInert(children)
}

Expand Down
4 changes: 2 additions & 2 deletions test/integration/build-output/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,10 +98,10 @@ describe('Build Output', () => {
expect(parseFloat(indexFirstLoad) - 62).toBeLessThanOrEqual(0)
expect(indexFirstLoad.endsWith('kB')).toBe(true)

expect(parseFloat(err404Size) - 3.6).toBeLessThanOrEqual(0)
expect(parseFloat(err404Size) - 3.7).toBeLessThanOrEqual(0)
expect(err404Size.endsWith('kB')).toBe(true)

expect(parseFloat(err404FirstLoad) - 65.2).toBeLessThanOrEqual(0)
expect(parseFloat(err404FirstLoad) - 68.1).toBeLessThanOrEqual(0)
expect(err404FirstLoad.endsWith('kB')).toBe(true)

expect(parseFloat(sharedByAll) - 61.8).toBeLessThanOrEqual(0)
Expand Down
42 changes: 31 additions & 11 deletions test/integration/font-optimization/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ function runTests() {
)
})

it('should minify the css', async () => {
it.skip('should minify the css', async () => {
const snapshotJson = JSON.parse(
await fs.readFile(join(__dirname, 'manifest-snapshot.json'), {
encoding: 'utf-8',
Expand All @@ -104,13 +104,9 @@ function runTests() {
})
}

describe.skip('Font optimization for SSR apps', () => {
describe('Font optimization for SSR apps', () => {
beforeAll(async () => {
await fs.writeFile(
nextConfig,
`module.exports = { experimental: {optimizeFonts: true} }`,
'utf8'
)
await fs.writeFile(nextConfig, `module.exports = {}`, 'utf8')

if (fs.pathExistsSync(join(appDir, '.next'))) {
await fs.remove(join(appDir, '.next'))
Expand All @@ -125,11 +121,11 @@ describe.skip('Font optimization for SSR apps', () => {
runTests()
})

describe.skip('Font optimization for serverless apps', () => {
describe('Font optimization for serverless apps', () => {
beforeAll(async () => {
await fs.writeFile(
nextConfig,
`module.exports = { target: 'serverless', experimental: {optimizeFonts: true} }`,
`module.exports = { target: 'serverless' }`,
'utf8'
)
await nextBuild(appDir)
Expand All @@ -142,11 +138,11 @@ describe.skip('Font optimization for serverless apps', () => {
runTests()
})

describe.skip('Font optimization for emulated serverless apps', () => {
describe('Font optimization for emulated serverless apps', () => {
beforeAll(async () => {
await fs.writeFile(
nextConfig,
`module.exports = { target: 'experimental-serverless-trace', experimental: {optimizeFonts: true} }`,
`module.exports = { target: 'experimental-serverless-trace' }`,
'utf8'
)
await nextBuild(appDir)
Expand All @@ -160,3 +156,27 @@ describe.skip('Font optimization for emulated serverless apps', () => {
})
runTests()
})

describe('Font optimization for unreachable font definitions.', () => {
beforeAll(async () => {
await fs.writeFile(nextConfig, `module.exports = { }`, 'utf8')
await nextBuild(appDir)
await fs.writeFile(
join(appDir, '.next', 'server', 'font-manifest.json'),
'[]',
'utf8'
)
appPort = await findPort()
app = await nextStart(appDir, appPort)
builtServerPagesDir = join(appDir, '.next', 'serverless')
builtPage = (file) => join(builtServerPagesDir, file)
})
afterAll(() => killApp(app))
it('should fallback to normal stylesheet if the contents of the fonts are unreachable', async () => {
const html = await renderViaHTTP(appPort, '/stars')
expect(await fsExists(builtPage('font-manifest.json'))).toBe(true)
expect(html).toContain(
'<link rel="stylesheet" href="https://fonts.googleapis.com/css2?family=Roboto:wght@700"/>'
)
})
})
5 changes: 0 additions & 5 deletions test/integration/script-loader/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,11 +112,6 @@ describe('Script Loader', () => {
`link[rel=stylesheet][href^="/_next/static/css"] ~ link[rel=preload][href="${src}"]`
).length
).toBeGreaterThan(0)
expect(
$(
`link[rel=stylesheet][href="https://fonts.googleapis.com/css?family=Voces"] ~ link[rel=preload][href="${src}"]`
).length
).toBeGreaterThan(0)

// Script is inserted before NextScripts
expect(
Expand Down