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

Separate Low Priority Files from Main Files #10756

Merged
merged 2 commits into from Feb 29, 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 9 additions & 13 deletions packages/next/build/webpack/plugins/build-manifest-plugin.ts
@@ -1,7 +1,6 @@
import devalue from 'devalue'
import { Compiler } from 'webpack'
import { RawSource } from 'webpack-sources'

import {
BUILD_MANIFEST,
CLIENT_STATIC_FILES_PATH,
Expand All @@ -10,19 +9,12 @@ import {
IS_BUNDLED_PAGE_REGEX,
ROUTE_NAME_REGEX,
} from '../../../next-server/lib/constants'

interface AssetMap {
devFiles: string[]
pages: {
'/_app': string[]
[s: string]: string[]
}
}
import { BuildManifest } from '../../../next-server/server/get-page-files'

// This function takes the asset map generated in BuildManifestPlugin and creates a
// reduced version to send to the client.
const generateClientManifest = (
assetMap: AssetMap,
assetMap: BuildManifest,
isModern: boolean
): string => {
const clientManifest: { [s: string]: string[] } = {}
Expand Down Expand Up @@ -68,7 +60,11 @@ export default class BuildManifestPlugin {
'NextJsBuildManifest',
(compilation, callback) => {
const { chunks } = compilation
const assetMap: AssetMap = { devFiles: [], pages: { '/_app': [] } }
const assetMap: BuildManifest = {
devFiles: [],
lowPriorityFiles: [],
pages: { '/_app': [] },
}

const mainJsChunk = chunks.find(
c => c.name === CLIENT_STATIC_FILES_RUNTIME_MAIN
Expand Down Expand Up @@ -141,11 +137,11 @@ export default class BuildManifestPlugin {
// as a dependency for the app. If the flag is false, the file won't be
// downloaded by the client.
if (this.clientManifest) {
assetMap.pages['/_app'].push(
assetMap.lowPriorityFiles.push(
`${CLIENT_STATIC_FILES_PATH}/${this.buildId}/_buildManifest.js`
)
if (this.modern) {
assetMap.pages['/_app'].push(
assetMap.lowPriorityFiles.push(
`${CLIENT_STATIC_FILES_PATH}/${this.buildId}/_buildManifest.module.js`
)
}
Expand Down
1 change: 1 addition & 0 deletions packages/next/next-server/lib/utils.ts
Expand Up @@ -153,6 +153,7 @@ export type DocumentProps = DocumentInitialProps & {
hasCssMode: boolean
devFiles: string[]
files: string[]
lowPriorityFiles: string[]
polyfillFiles: string[]
dynamicImports: ManifestItem[]
assetPrefix?: string
Expand Down
2 changes: 2 additions & 0 deletions packages/next/next-server/server/get-page-files.ts
Expand Up @@ -2,7 +2,9 @@ import { normalizePagePath } from './normalize-page-path'

export type BuildManifest = {
devFiles: string[]
lowPriorityFiles: string[]
pages: {
'/_app': string[]
[page: string]: string[]
}
}
Expand Down
9 changes: 7 additions & 2 deletions packages/next/next-server/server/render.tsx
Expand Up @@ -174,6 +174,7 @@ function renderDocument(
staticMarkup,
devFiles,
files,
lowPriorityFiles,
polyfillFiles,
dynamicImports,
htmlProps,
Expand All @@ -191,8 +192,9 @@ function renderDocument(
hybridAmp: boolean
dynamicImportsIds: string[]
dynamicImports: ManifestItem[]
files: string[]
devFiles: string[]
files: string[]
lowPriorityFiles: string[]
polyfillFiles: string[]
htmlProps: any
bodyTags: any
Expand Down Expand Up @@ -229,6 +231,7 @@ function renderDocument(
staticMarkup,
devFiles,
files,
lowPriorityFiles,
polyfillFiles,
dynamicImports,
assetPrefix,
Expand Down Expand Up @@ -579,6 +582,7 @@ export async function renderToHTML(
...getPageFiles(buildManifest, pathname),
]),
]
const lowPriorityFiles = buildManifest.lowPriorityFiles
const polyfillFiles = getPageFiles(buildManifest, '/_polyfills')

const renderElementToString = staticMarkup
Expand Down Expand Up @@ -674,8 +678,9 @@ export async function renderToHTML(
hybridAmp,
dynamicImportsIds,
dynamicImports,
files,
devFiles,
files,
lowPriorityFiles,
polyfillFiles,
})

Expand Down
25 changes: 5 additions & 20 deletions packages/next/pages/_document.tsx
Expand Up @@ -44,10 +44,6 @@ function getOptionalModernScriptVariant(path: string) {
return path
}

function isLowPriority(file: string) {
return file.includes('_buildManifest')
}

/**
* `Document` component handles the initial `document` markup and renders only on the server side.
* Commonly used for implementing server side rendering for `css-in-js` libraries.
Expand Down Expand Up @@ -260,13 +256,7 @@ export class Head extends Component<
// `dynamicImports` will contain both `.js` and `.module.js` when
// the feature is enabled. This clause will filter down to the
// modern variants only.
//
// Also filter out low priority files because they should not be
// preloaded for performance reasons.
return (
file.endsWith(getOptionalModernScriptVariant('.js')) &&
!isLowPriority(file)
)
return file.endsWith(getOptionalModernScriptVariant('.js'))
})
: []

Expand Down Expand Up @@ -589,17 +579,12 @@ export class NextScript extends Component<OriginProps> {
}

getScripts() {
const { assetPrefix, files } = this.context._documentProps
if (!files || files.length === 0) {
return null
}
const { assetPrefix, files, lowPriorityFiles } = this.context._documentProps
const { _devOnlyInvalidateCacheQueryString } = this.context

const normalScripts = files.filter(
file => file.endsWith('.js') && !isLowPriority(file)
)
const lowPriorityScripts = files.filter(
file => file.endsWith('.js') && isLowPriority(file)
const normalScripts = files?.filter(file => file.endsWith('.js'))
const lowPriorityScripts = lowPriorityFiles?.filter(file =>
file.endsWith('.js')
)

return [...normalScripts, ...lowPriorityScripts].map(file => {
Expand Down
19 changes: 16 additions & 3 deletions test/integration/chunking/test/index.test.js
@@ -1,12 +1,12 @@
/* eslint-env jest */
/* global jasmine */
import { join } from 'path'
import cheerio from 'cheerio'
import express from 'express'
import { access, readdir, readFile, unlink } from 'fs-extra'
import http from 'http'
import { nextBuild, nextServer, promiseCall, stopApp } from 'next-test-utils'
import { readdir, readFile, unlink, access } from 'fs-extra'
import cheerio from 'cheerio'
import webdriver from 'next-webdriver'
import { join } from 'path'

jasmine.DEFAULT_TIMEOUT_INTERVAL = 1000 * 60 * 1

Expand Down Expand Up @@ -90,6 +90,19 @@ describe('Chunking', () => {
).toBe(false)
})

it('should execute the build manifest', async () => {
const indexPage = await readFile(
join(appDir, '.next', 'server', 'static', buildId, 'pages', 'index.html')
)

const $ = cheerio.load(indexPage)
expect(
Array.from($('script'))
.map(e => e.attribs.src)
.some(entry => entry && entry.includes('_buildManifest'))
).toBe(true)
})

it('should not include more than one instance of react-dom', async () => {
const misplacedReactDom = stats.chunks.some(chunk => {
if (chunk.names.includes('framework')) {
Expand Down