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

Hmr DX improvements #29753

Merged
merged 2 commits into from
Oct 9, 2021
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
129 changes: 78 additions & 51 deletions packages/next/build/output/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ let previousClient: import('webpack').Compiler | null = null
let previousServer: import('webpack').Compiler | null = null

type CompilerDiagnostics = {
modules: number
errors: string[] | null
warnings: string[] | null
}
Expand All @@ -36,31 +37,10 @@ export type AmpPageStatus = {
type BuildStatusStore = {
client: WebpackStatus
server: WebpackStatus
trigger: string | undefined
amp: AmpPageStatus
}

// eslint typescript has a bug with TS enums
/* eslint-disable no-shadow */
enum WebpackStatusPhase {
COMPILING = 1,
COMPILED_WITH_ERRORS = 2,
COMPILED_WITH_WARNINGS = 4,
COMPILED = 5,
}

function getWebpackStatusPhase(status: WebpackStatus): WebpackStatusPhase {
if (status.loading) {
return WebpackStatusPhase.COMPILING
}
if (status.errors) {
return WebpackStatusPhase.COMPILED_WITH_ERRORS
}
if (status.warnings) {
return WebpackStatusPhase.COMPILED_WITH_WARNINGS
}
return WebpackStatusPhase.COMPILED
}

export function formatAmpMessages(amp: AmpPageStatus) {
let output = chalk.bold('Amp Validation') + '\n\n'
let messages: string[][] = []
Expand Down Expand Up @@ -118,45 +98,66 @@ export function formatAmpMessages(amp: AmpPageStatus) {
const buildStore = createStore<BuildStatusStore>()

buildStore.subscribe((state) => {
const { amp, client, server } = state

const [{ status }] = [
{ status: client, phase: getWebpackStatusPhase(client) },
{ status: server, phase: getWebpackStatusPhase(server) },
].sort((a, b) => a.phase.valueOf() - b.phase.valueOf())
const { amp, client, server, trigger } = state

const { bootstrap: bootstrapping, appUrl } = consoleStore.getState()
if (bootstrapping && status.loading) {
if (bootstrapping && (client.loading || server.loading)) {
return
}

if (client.loading || server.loading) {
consoleStore.setState(
{
bootstrap: false,
appUrl: appUrl!,
loading: true,
trigger,
} as OutputState,
true
)
return
}

let partialState: Partial<OutputState> = {
bootstrap: false,
appUrl: appUrl!,
loading: false,
typeChecking: false,
modules: client.modules + server.modules,
}

if (status.loading) {
if (client.errors) {
// Show only client errors
consoleStore.setState(
{ ...partialState, loading: true } as OutputState,
{
...partialState,
errors: client.errors,
warnings: null,
} as OutputState,
true
)
} else if (server.errors) {
// Show only server errors
consoleStore.setState(
{
...partialState,
errors: server.errors,
warnings: null,
} as OutputState,
true
)
} else {
let { errors, warnings } = status

if (errors == null) {
if (Object.keys(amp).length > 0) {
warnings = (warnings || []).concat(formatAmpMessages(amp) || [])
if (!warnings.length) warnings = null
}
}
// Show warnings from all of them
const warnings = [
...(client.warnings || []),
...(server.warnings || []),
...((Object.keys(amp).length > 0 && formatAmpMessages(amp)) || []),
]

consoleStore.setState(
{
...partialState,
loading: false,
typeChecking: false,
errors,
warnings,
errors: null,
warnings: warnings.length === 0 ? null : warnings,
} as OutputState,
true
)
Expand Down Expand Up @@ -200,6 +201,7 @@ export function watchCompilers(
buildStore.setState({
client: { loading: true },
server: { loading: true },
trigger: 'initial',
})

function tapCompiler(
Expand All @@ -213,7 +215,7 @@ export function watchCompilers(

compiler.hooks.done.tap(
`NextJsDone-${key}`,
(stats: import('webpack').Stats) => {
(stats: import('webpack5').Stats) => {
buildStore.setState({ amp: {} })

const { errors, warnings } = formatWebpackMessages(
Expand All @@ -225,20 +227,45 @@ export function watchCompilers(

onEvent({
loading: false,
modules: stats.compilation.modules.size,
errors: hasErrors ? errors : null,
warnings: hasWarnings ? warnings : null,
})
}
)
}

tapCompiler('client', client, (status) =>
buildStore.setState({ client: status })
)
tapCompiler('server', server, (status) =>
buildStore.setState({ server: status })
)
tapCompiler('client', client, (status) => {
if (!status.loading && !buildStore.getState().server.loading) {
buildStore.setState({
client: status,
trigger: undefined,
})
} else {
buildStore.setState({
client: status,
})
}
})
tapCompiler('server', server, (status) => {
if (!status.loading && !buildStore.getState().client.loading) {
buildStore.setState({
server: status,
trigger: undefined,
})
} else {
buildStore.setState({
server: status,
})
}
})

previousClient = client
previousServer = server
}

export function reportTrigger(trigger: string) {
buildStore.setState({
trigger,
})
}
22 changes: 18 additions & 4 deletions packages/next/build/output/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,14 @@ import * as Log from './log'
export type OutputState =
| { bootstrap: true; appUrl: string | null; bindAddr: string | null }
| ({ bootstrap: false; appUrl: string | null; bindAddr: string | null } & (
| { loading: true }
| {
loading: true
trigger: string | undefined
}
| {
loading: false
typeChecking: boolean
modules: number
errors: string[] | null
warnings: string[] | null
}
Expand Down Expand Up @@ -49,11 +53,16 @@ store.subscribe((state) => {
if (state.appUrl) {
Log.ready(`started server on ${state.bindAddr}, url: ${state.appUrl}`)
}
if (startTime === 0) startTime = Date.now()
return
}

if (state.loading) {
Log.wait('compiling...')
if (state.trigger) {
Log.wait(`compiling ${state.trigger}...`)
} else {
Log.wait('compiling...')
}
if (startTime === 0) startTime = Date.now()
return
}
Expand Down Expand Up @@ -89,6 +98,11 @@ store.subscribe((state) => {
time > 2000 ? ` in ${Math.round(time / 100) / 10} s` : ` in ${time} ms`
}

let modulesMessage = ''
if (state.modules) {
modulesMessage = ` (${state.modules} modules)`
}

if (state.warnings) {
Log.warn(state.warnings.join('\n\n'))
// Ensure traces are flushed after each compile in development mode
Expand All @@ -98,12 +112,12 @@ store.subscribe((state) => {

if (state.typeChecking) {
Log.info(
`bundled successfully${timeMessage}, waiting for typecheck results...`
`bundled successfully${timeMessage}${modulesMessage}, waiting for typecheck results...`
)
return
}

Log.event(`compiled successfully${timeMessage}`)
Log.event(`compiled successfully${timeMessage}${modulesMessage}`)
// Ensure traces are flushed after each compile in development mode
flushAllTraces()
})
10 changes: 5 additions & 5 deletions packages/next/server/dev/on-demand-entry-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@ import { IncomingMessage, ServerResponse } from 'http'
import { join, posix } from 'path'
import { parse } from 'url'
import { webpack } from 'next/dist/compiled/webpack/webpack'
import * as Log from '../../build/output/log'
import { normalizePagePath, normalizePathSep } from '../normalize-page-path'
import { pageNotFoundError } from '../require'
import { findPageFile } from '../lib/find-page-file'
import getRouteFromEntrypoint from '../get-route-from-entrypoint'
import { API_ROUTE } from '../../lib/constants'
import { reportTrigger } from '../../build/output'

export const ADDED = Symbol('added')
export const BUILDING = Symbol('building')
Expand Down Expand Up @@ -228,12 +228,12 @@ export default function onDemandEntryHandler(
: Promise.all([addPageEntry('client'), addPageEntry('server')])

if (entriesChanged) {
Log.event(
reportTrigger(
isApiRoute
? `build page: ${normalizedPage} (server only)`
? `${normalizedPage} (server only)`
: clientOnly
? `build page: ${normalizedPage} (client only)`
: `build page: ${normalizedPage}`
? `${normalizedPage} (client only)`
: normalizedPage
)
invalidator.invalidate()
}
Expand Down
2 changes: 1 addition & 1 deletion test/integration/amphtml/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -520,7 +520,7 @@ describe('AMP Usage', () => {

it('should not contain missing files warning', async () => {
expect(output).toContain('compiled successfully')
expect(output).toContain('build page: /only-amp')
expect(output).toContain('compiling /only-amp')
expect(output).not.toContain('Could not find files for')
})
})
Expand Down
7 changes: 7 additions & 0 deletions test/integration/gssp-ssr-change-reloading/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ describe('GS(S)P Server-Side Change Reloading', () => {

it('should not reload page when client-side is changed too GSP', async () => {
const browser = await webdriver(appPort, '/gsp-blog/first')
await check(() => browser.elementByCss('#change').text(), 'change me')
await browser.eval(() => (window.beforeChange = 'hi'))

const props = JSON.parse(await browser.elementByCss('#props').text())
Expand Down Expand Up @@ -145,6 +146,7 @@ describe('GS(S)P Server-Side Change Reloading', () => {

it('should not reload page when client-side is changed too GSSP', async () => {
const browser = await webdriver(appPort, '/gssp-blog/first')
await check(() => browser.elementByCss('#change').text(), 'change me')
await browser.eval(() => (window.beforeChange = 'hi'))

const props = JSON.parse(await browser.elementByCss('#props').text())
Expand All @@ -165,6 +167,11 @@ describe('GS(S)P Server-Side Change Reloading', () => {

it('should update page when getServerSideProps is changed only', async () => {
const browser = await webdriver(appPort, '/gssp-blog/first')
await check(
async () =>
JSON.parse(await browser.elementByCss('#props').text()).count + '',
'1'
)
await browser.eval(() => (window.beforeChange = 'hi'))

const props = JSON.parse(await browser.elementByCss('#props').text())
Expand Down