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

Font optimizations #14746

Merged
merged 74 commits into from
Jul 28, 2020
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
74 commits
Select commit Hold shift + click to select a range
eff6343
adding a font gatherer webpack plugin
prateekbh Jun 15, 2020
9ed7962
fixing dep
prateekbh Jun 15, 2020
3b85140
Initial commit with framework for post processing middleware
atcastle Jun 18, 2020
3934f53
Merge branch 'post-process-optimization' of https://github.com/azukar…
prateekbh Jun 18, 2020
d5ed404
generating the style tags
prateekbh Jun 23, 2020
39fcfcd
WIP font optimizations
prateekbh Jun 24, 2020
8a932d1
working static pages font replacement
prateekbh Jun 25, 2020
126f904
minifying stylesheets
prateekbh Jun 25, 2020
069cbfa
Merge branch 'canary' of https://github.com/zeit/next.js into font-op…
prateekbh Jun 25, 2020
27bdbfe
Merge branch 'canary' of https://github.com/zeit/next.js into post-pr…
prateekbh Jun 25, 2020
6a14652
Merge branch 'post-process-optimization' into font-optims
prateekbh Jun 25, 2020
d5137c3
WIP font-optimization changes
prateekbh Jun 30, 2020
7a79523
Merge branch 'canary' of https://github.com/zeit/next.js into font-op…
prateekbh Jun 30, 2020
0f89a6c
adding comments
prateekbh Jun 30, 2020
2f45a7b
switching from filter to map
prateekbh Jun 30, 2020
6f9e1e7
fixing server path
prateekbh Jun 30, 2020
210a5cf
Fixing serverless builds
prateekbh Jun 30, 2020
ca93eba
reverting unwated change
prateekbh Jun 30, 2020
0bad5cb
removing commented code
prateekbh Jun 30, 2020
463ca85
adding experimental flag
prateekbh Jun 30, 2020
004ebcb
code resuability
prateekbh Jun 30, 2020
6acdce5
renaming flags
prateekbh Jun 30, 2020
2289980
renaming flags
prateekbh Jun 30, 2020
0d0c96d
guarding behind a flag
prateekbh Jul 1, 2020
45e7929
removing Promise.allSettled
prateekbh Jul 1, 2020
2602b31
Bug fixes and adding tests
prateekbh Jul 2, 2020
5cde0b2
sending experimental flags
prateekbh Jul 3, 2020
fcf567c
bug fixes and adding tests
prateekbh Jul 3, 2020
f70f936
adding serverless tests
prateekbh Jul 3, 2020
3ef9d55
minor improvement
prateekbh Jul 3, 2020
98e41c7
fixing experimental flag
prateekbh Jul 3, 2020
b8917c5
adding tests for serverless apps
prateekbh Jul 4, 2020
0dabf09
bug fix and adding tests for serverless
prateekbh Jul 4, 2020
9bfd563
Merge branch 'canary' of https://github.com/zeit/next.js into font-op…
prateekbh Jul 4, 2020
e17d425
uniform logic for inert font stylesheets
prateekbh Jul 5, 2020
c397f6c
sending process.env value to SSR case
prateekbh Jul 5, 2020
5f7ddd1
updating lock file
prateekbh Jul 5, 2020
a0a9c14
Merge branch 'canary' into font-optims
prateekbh Jul 5, 2020
c7fe9e9
Merge branch 'canary' of https://github.com/zeit/next.js into font-op…
prateekbh Jul 5, 2020
a21bba1
Merge branch 'font-optims' of https://github.com/azukaru/next.js into…
prateekbh Jul 5, 2020
d2be443
type fixes
prateekbh Jul 5, 2020
b7cd379
bug fix in test
prateekbh Jul 5, 2020
05cc713
bug fixes
prateekbh Jul 6, 2020
623a965
Merge branch 'canary' of https://github.com/zeit/next.js into font-op…
prateekbh Jul 6, 2020
dc83f9d
fixing serializable value bug
prateekbh Jul 6, 2020
1e23ca3
vanity changes
prateekbh Jul 6, 2020
97f89fe
query bug fixes
prateekbh Jul 6, 2020
cb3ff87
Merge branch 'canary' of https://github.com/zeit/next.js into font-op…
prateekbh Jul 6, 2020
3eaade2
moving ast types to dep
prateekbh Jul 6, 2020
6000fae
shifting font urls to a constant
prateekbh Jul 7, 2020
20ab602
removing dependency from recast
prateekbh Jul 7, 2020
91b5bb4
Merge branch 'canary' of https://github.com/zeit/next.js into font-op…
prateekbh Jul 7, 2020
cb5e320
fixing flag name and adding tests
prateekbh Jul 8, 2020
cf4d9d1
Merge branch 'canary' into font-optims
prateekbh Jul 8, 2020
bf09fcb
addressing comments
prateekbh Jul 9, 2020
bd2f156
Merge branch 'canary' of https://github.com/zeit/next.js into font-op…
prateekbh Jul 9, 2020
815d279
adding tests for emulated serverless builds
prateekbh Jul 15, 2020
58ad186
removes the need to fetch font from network in render and test fixes
prateekbh Jul 15, 2020
8de7630
Merge branch 'canary' of https://github.com/zeit/next.js into font-op…
prateekbh Jul 15, 2020
6d600ab
fixing type issues
prateekbh Jul 15, 2020
477937b
Merge branch 'canary' into font-optims
prateekbh Jul 15, 2020
a462650
fix for ie 11
prateekbh Jul 16, 2020
3762863
Merge branch 'font-optims' of https://github.com/azukaru/next.js into…
prateekbh Jul 16, 2020
af2c9d6
Merge branch 'canary' of https://github.com/zeit/next.js into font-op…
prateekbh Jul 16, 2020
45a90ed
removed the const due to tree shaking problem
prateekbh Jul 17, 2020
b3f7aea
Merge branch 'canary' of https://github.com/zeit/next.js into font-op…
prateekbh Jul 17, 2020
03eb098
increasing the error.js size as it now has filter logic for font opti…
prateekbh Jul 17, 2020
7f5680a
Merge branch 'canary' into font-optims
prateekbh Jul 17, 2020
2d77d04
Merge branch 'canary' of https://github.com/zeit/next.js into font-op…
prateekbh Jul 18, 2020
90f41fb
Update index.test.js
prateekbh Jul 20, 2020
0b6ee6b
Merge branch 'canary' of https://github.com/zeit/next.js into font-op…
prateekbh Jul 20, 2020
eeb5375
Merge branch 'font-optims' of https://github.com/azukaru/next.js into…
prateekbh Jul 20, 2020
9f7f904
Merge branch 'canary' of https://github.com/zeit/next.js into font-op…
prateekbh Jul 28, 2020
eb7f10e
Merge branch 'canary' into font-optims
prateekbh Jul 28, 2020
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
3 changes: 2 additions & 1 deletion packages/next/build/webpack-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ import WebpackConformancePlugin, {
ReactSyncScriptsConformanceCheck,
} from './webpack/plugins/webpack-conformance-plugin'
import { WellKnownErrorsPlugin } from './webpack/plugins/wellknown-errors-plugin'

import FontStylesheetGatheringPlugin from './webpack/plugins/font-stylesheet-gathering-plugin'
type ExcludesFalse = <T>(x: T | false) => x is T

const isWebpack5 = parseInt(webpack.version!) === 5
Expand Down Expand Up @@ -961,6 +961,7 @@ export default async function getBaseWebpackConfig(
inputChunkName.replace(/\.js$/, '.module.js'),
})
})(),
!dev && isServer && new FontStylesheetGatheringPlugin(),
config.experimental.conformance &&
!dev &&
new WebpackConformancePlugin({
Expand Down
12 changes: 12 additions & 0 deletions packages/next/build/webpack/loaders/next-serverless-loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ const nextServerlessLoader: loader.Loader = function () {
import initServer from 'next-plugin-loader?middleware=on-init-server!'
import onError from 'next-plugin-loader?middleware=on-error-server!'
import 'next/dist/next-server/server/node-polyfill-fetch'
import { requireFontManifest } from 'next/dist/next-server/server/require'
const {isResSent} = require('next/dist/next-server/lib/utils');

${envLoading}
Expand Down Expand Up @@ -401,7 +402,18 @@ const nextServerlessLoader: loader.Loader = function () {
// make sure to set renderOpts to the correct params e.g. _params
// if provided from worker or params if we're parsing them here
renderOpts.params = _params || params
renderOpts.getFontDefinition = (fontURL) => {
console.log('will dist dirrrr', "${distDir}", "${canonicalBase}", "${basePath}");
const manifest = requireFontManifest("${distDir}", true)
timneutkens marked this conversation as resolved.
Show resolved Hide resolved
let fontContent = ''
manifest.forEach((font) => {
if (font && font.url === fontURL) {
fontContent = font.content
}
})

return fontContent
}
const isFallback = parsedUrl.query.__nextFallback

const previewData = tryGetPreviewData(req, res, options.previewProps)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
// eslint-disable-next-line import/no-extraneous-dependencies
import { NodePath } from 'ast-types/lib/node-path'
import { visit } from 'next/dist/compiled/recast'
import { compilation as CompilationType, Compiler } from 'webpack'
import { namedTypes } from 'ast-types'
import { RawSource } from 'webpack-sources'

const https = require('https')

interface VisitorMap {
[key: string]: (path: NodePath) => void
}

export default class FontStylesheetGatheringPlugin {
compiler?: Compiler
gatheredStylesheets: Array<string> = []

private parserHandler = (
factory: CompilationType.NormalModuleFactory
): void => {
const JS_TYPES = ['auto', 'esm', 'dynamic']
// Do an extra walk per module and add interested visitors to the walk.
for (const type of JS_TYPES) {
factory.hooks.parser
.for('javascript/' + type)
.tap(this.constructor.name, (parser) => {
var that = this
parser.hooks.program.tap(this.constructor.name, (ast: any) => {
// We will only optimize fonts from first party code.
if (parser?.state?.module?.resource.includes('node_modules')) {
return
}
visit(ast, {
visitCallExpression: function (path) {
Copy link
Member

Choose a reason for hiding this comment

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

I believe you can tap into the callExpression directly instead of manually traversing the AST:
https://github.com/webpack/webpack/blob/master/lib/javascript/JavascriptParser.js#L236-L260

Copy link
Contributor Author

@prateekbh prateekbh Jul 7, 2020

Choose a reason for hiding this comment

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

I agree. This might also let us remove the logic from both next/head as we could just make the link tags inert here itself.
But I could never get this to work hence added recast at the time of conformance work.
FWIW, this is the code i tried

parser.hooks.expression(or call).for('__jsx').tap('FontStylesheetGatheringPlugin', expression => {
 console.log(parser.state.module.resource)
})

^ would let us capture the call creating the link tag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update:https://twitter.com/_prateekbh/status/1280603310211215361
Done 👍🏻
its not the most readable/straightforward thing, but it'll definitely be faster

const { node }: { node: namedTypes.CallExpression } = path
if (!node.arguments || node.arguments.length < 2) {
return false
}
if (isNodeCreatingLinkElement(node)) {
const propsNode = node
.arguments[1] as namedTypes.ObjectExpression
if (!propsNode.properties) {
return false
}
const props: {
[key: string]: string
} = propsNode.properties.reduce(
(originalProps, prop: any) => {
// todo check the type of prop
// @ts-ignore
originalProps[prop.key.name] = prop.value.value
return originalProps
},
{}
)

if (
!props.rel ||
props.rel !== 'stylesheet' ||
!props.href ||
!props.href.startsWith('https://')
) {
return false
}
that.gatheredStylesheets.push(props.href)
}
this.traverse(path)
return false
},
})
})
})
}
}

public apply(compiler: Compiler) {
this.compiler = compiler
compiler.hooks.normalModuleFactory.tap(
this.constructor.name,
this.parserHandler
)
compiler.hooks.make.tapAsync(this.constructor.name, (compilation, cb) => {
compilation.hooks.finishModules.tapAsync(
this.constructor.name,
async (_, modulesFinished) => {
const allContent = this.gatheredStylesheets.map((url) => getFile(url))
const manifestContent = (await Promise.allSettled(allContent)).map(
(promise) => promise.value
)
compilation.assets['font-manifest.json'] = new RawSource(
JSON.stringify(manifestContent, null, ' ')
)
modulesFinished()
}
)
cb()
})
}
}

function isNodeCreatingLinkElement(node: namedTypes.CallExpression) {
const callee = node.callee as namedTypes.Identifier
if (callee.type !== 'Identifier') {
return false
}
const componentNode = node.arguments[0] as namedTypes.Literal
if (componentNode.type !== 'Literal') {
return false
}
// Next has pragma: __jsx.
return callee.name === '__jsx' && componentNode.value === 'link'
}

function getFile(url: string): Promise<string> {
return new Promise((resolve) => {
let rawData: any = ''
https.get(
url,
{
headers: {
'user-agent':
'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/83.0.4103.61 Safari/537.36',
},
},
(res: any) => {
res.on('data', (chunk: any) => {
rawData += chunk
})
res.on('end', () => {
resolve({
url,
content: rawData.toString('utf8'),
})
})
}
)
})
}
27 changes: 25 additions & 2 deletions packages/next/export/worker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import 'next/dist/next-server/server/node-polyfill-fetch'
import { IncomingMessage, ServerResponse } from 'http'
import { ComponentType } from 'react'
import { GetStaticProps } from '../types'
import { requireFontManifest } from '../next-server/server/require'

const envConfig = require('../next-server/lib/runtime-config')

Expand Down Expand Up @@ -60,6 +61,7 @@ interface RenderOpts {
ampSkipValidation?: boolean
hybridAmp?: boolean
inAmpMode?: boolean
getFontDefinition?: (url: string) => string
}

type ComponentModule = ComponentType<{}> & {
Expand All @@ -83,6 +85,18 @@ export default async function exportPage({
ampValidations: [],
}

const getFontDefinition = (fontURL: string) => {
const manifest = requireFontManifest(distDir, serverless)
let fontContent = ''
manifest.forEach((font: any) => {
if (font && font.url === fontURL) {
fontContent = font.content
}
})

return fontContent
}

try {
const { query: originalQuery = {} } = pathMap
const { page } = pathMap
Expand Down Expand Up @@ -213,7 +227,10 @@ export default async function exportPage({
'export',
{ ampPath },
// @ts-ignore
params
{
...params,
getFontDefinition,
}
)
curRenderOpts = result.renderOpts || {}
html = result.html
Expand Down Expand Up @@ -246,7 +263,13 @@ export default async function exportPage({
html = components.Component
queryWithAutoExportWarn()
} else {
curRenderOpts = { ...components, ...renderOpts, ampPath, params }
curRenderOpts = {
...components,
...renderOpts,
ampPath,
params,
getFontDefinition,
}
// @ts-ignore
html = await renderMethod(req, res, page, query, curRenderOpts)
prateekbh marked this conversation as resolved.
Show resolved Hide resolved
}
Expand Down
1 change: 1 addition & 0 deletions packages/next/next-server/lib/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ export const EXPORT_DETAIL = 'export-detail.json'
export const PRERENDER_MANIFEST = 'prerender-manifest.json'
export const ROUTES_MANIFEST = 'routes-manifest.json'
export const REACT_LOADABLE_MANIFEST = 'react-loadable-manifest.json'
export const FONT_MANIFEST = 'font-manifest.json'
Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding a new manifest it'd easier to reuse the build-manifest I think 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

May be, however I am afraid that by writing build-manifest from multiple plugins they might overwrite each other?

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't have to be multiple plugins though, we can share information into the build manifest 👍

The reason using build manifest is better is that it's automatically made available to rendering and _document, meaning you don't have to change the rendering code as much as you have to when introducing a new file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found that BuildManifestPlugin is built on client run of webpack. This is a problem because FontStylesheetGatheringPlugin runs on the server build of webpack.
The reason for keeping it on server side run is that otherwise it does not build the user defined _document.jsx which is a common place to define stylesheets and fonts and seems to be building only for server run of webpack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@timneutkens would there be a way to generate this on server run and consolidate during client bundle of webpack?

export const SERVER_DIRECTORY = 'server'
export const SERVERLESS_DIRECTORY = 'serverless'
export const CONFIG_FILE = 'next.config.js'
Expand Down
Loading