From 76dd392c1afabbfeb51cf653e441bd4c5596f82c Mon Sep 17 00:00:00 2001 From: Joe Haddad Date: Tue, 25 Jun 2019 15:59:37 -0400 Subject: [PATCH 1/4] Asynchronously check for type errors --- packages/next/build/output/index.ts | 128 ++++++++++++++++-- packages/next/build/output/store.ts | 12 +- packages/next/build/webpack-config.ts | 2 +- .../plugins/fork-ts-checker-watcher-hook.ts | 27 ---- packages/next/server/hot-reloader.js | 14 +- 5 files changed, 130 insertions(+), 53 deletions(-) delete mode 100644 packages/next/build/webpack/plugins/fork-ts-checker-watcher-hook.ts diff --git a/packages/next/build/output/index.ts b/packages/next/build/output/index.ts index 53d33573dd8a9..b093336830133 100644 --- a/packages/next/build/output/index.ts +++ b/packages/next/build/output/index.ts @@ -5,6 +5,9 @@ import stripAnsi from 'strip-ansi' import formatWebpackMessages from '../../client/dev/error-overlay/format-webpack-messages' import { OutputState, store as consoleStore } from './store' +import forkTsCheckerWebpackPlugin from 'fork-ts-checker-webpack-plugin' +import { NormalizedMessage } from 'fork-ts-checker-webpack-plugin/lib/NormalizedMessage' +import { createCodeframeFormatter } from 'fork-ts-checker-webpack-plugin/lib/formatter/codeframeFormatter' export function startedDevelopmentServer(appUrl: string) { consoleStore.setState({ appUrl }) @@ -13,13 +16,17 @@ export function startedDevelopmentServer(appUrl: string) { let previousClient: any = null let previousServer: any = null +type CompilerDiagnostics = { + errors: string[] | null + warnings: string[] | null +} + type WebpackStatus = | { loading: true } - | { + | ({ loading: false - errors: string[] | null - warnings: string[] | null - } + typeChecking: boolean + } & CompilerDiagnostics) type AmpStatus = { message: string @@ -41,8 +48,9 @@ type BuildStatusStore = { enum WebpackStatusPhase { COMPILING = 1, COMPILED_WITH_ERRORS = 2, - COMPILED_WITH_WARNINGS = 3, - COMPILED = 4, + TYPE_CHECKING = 3, + COMPILED_WITH_WARNINGS = 4, + COMPILED = 5, } function getWebpackStatusPhase(status: WebpackStatus): WebpackStatusPhase { @@ -52,6 +60,9 @@ function getWebpackStatusPhase(status: WebpackStatus): WebpackStatusPhase { if (status.errors) { return WebpackStatusPhase.COMPILED_WITH_ERRORS } + if (status.typeChecking) { + return WebpackStatusPhase.TYPE_CHECKING + } if (status.warnings) { return WebpackStatusPhase.COMPILED_WITH_WARNINGS } @@ -125,14 +136,36 @@ buildStore.subscribe(state => { true ) } else { - let { errors, warnings } = status + let { errors, warnings, typeChecking } = status + + if (errors == null) { + if (typeChecking) { + consoleStore.setState( + { + ...partialState, + loading: false, + typeChecking: true, + errors, + warnings, + } as OutputState, + true + ) + return + } - if (errors == null && Object.keys(amp).length > 0) { - warnings = (warnings || []).concat(formatAmpMessages(amp)) + if (Object.keys(amp).length > 0) { + warnings = (warnings || []).concat(formatAmpMessages(amp)) + } } consoleStore.setState( - { ...partialState, loading: false, errors, warnings } as OutputState, + { + ...partialState, + loading: false, + typeChecking: false, + errors, + warnings, + } as OutputState, true ) } @@ -162,7 +195,11 @@ export function ampValidation( }) } -export function watchCompiler(client: any, server: any) { +export function watchCompilers( + client: any, + server: any, + enableTypeCheckingOnClient: boolean +) { if (previousClient === client && previousServer === server) { return } @@ -175,12 +212,49 @@ export function watchCompiler(client: any, server: any) { function tapCompiler( key: string, compiler: any, + hasTypeChecking: boolean, onEvent: (status: WebpackStatus) => void ) { compiler.hooks.invalid.tap(`NextJsInvalid-${key}`, () => { onEvent({ loading: true }) }) + let tsMessagesPromise: Promise + let tsMessagesResolver: (diagnostics: CompilerDiagnostics) => void + + if (hasTypeChecking) { + const typescriptFormatter = createCodeframeFormatter({}) + + compiler.hooks.beforeCompile.tap(`NextJs-${key}-StartTypeCheck`, () => { + tsMessagesPromise = new Promise(resolve => { + tsMessagesResolver = msgs => resolve(msgs) + }) + }) + + forkTsCheckerWebpackPlugin + .getCompilerHooks(compiler) + .receive.tap( + `NextJs-${key}-afterTypeScriptCheck`, + (diagnostics: NormalizedMessage[], lints: NormalizedMessage[]) => { + const allMsgs = [...diagnostics, ...lints] + const format = (message: NormalizedMessage) => + typescriptFormatter(message, true) + + const errors = allMsgs + .filter(msg => msg.severity === 'error') + .map(format) + const warnings = allMsgs + .filter(msg => msg.severity === 'warning') + .map(format) + + tsMessagesResolver({ + errors: errors.length ? errors : null, + warnings: warnings.length ? warnings : null, + }) + } + ) + } + compiler.hooks.done.tap(`NextJsDone-${key}`, (stats: any) => { buildStore.setState({ amp: {} }) @@ -188,18 +262,42 @@ export function watchCompiler(client: any, server: any) { stats.toJson({ all: false, warnings: true, errors: true }) ) + const hasErrors = errors && errors.length + const hasWarnings = warnings && warnings.length + onEvent({ loading: false, - errors: errors && errors.length ? errors : null, - warnings: warnings && warnings.length ? warnings : null, + typeChecking: hasTypeChecking, + errors: hasErrors ? errors : null, + warnings: hasWarnings ? warnings : null, }) + + const typePromise = tsMessagesPromise + + if (!hasErrors && typePromise) { + typePromise.then(typeMessages => { + if (typePromise !== tsMessagesPromise) { + // a new compilation started so we don't care about this + return + } + + onEvent({ + loading: false, + typeChecking: false, + errors: typeMessages.errors, + warnings: hasWarnings + ? [...warnings, ...(typeMessages.warnings || [])] + : typeMessages.warnings, + }) + }) + } }) } - tapCompiler('client', client, status => + tapCompiler('client', client, enableTypeCheckingOnClient, status => buildStore.setState({ client: status }) ) - tapCompiler('server', server, status => + tapCompiler('server', server, false, status => buildStore.setState({ server: status }) ) diff --git a/packages/next/build/output/store.ts b/packages/next/build/output/store.ts index f543cff715b53..5dc541526b154 100644 --- a/packages/next/build/output/store.ts +++ b/packages/next/build/output/store.ts @@ -9,6 +9,7 @@ export type OutputState = | { loading: true } | { loading: false + typeChecking: boolean errors: string[] | null warnings: string[] | null } @@ -76,8 +77,13 @@ store.subscribe(state => { return } - Log.ready('compiled successfully') - if (state.appUrl) { - Log.info(`ready on ${state.appUrl}`) + if (state.typeChecking) { + Log.info('bundled successfully, waiting for typecheck results ...') + return } + + Log.ready( + 'compiled successfully' + + (state.appUrl ? ` (ready on ${state.appUrl})` : '') + ) }) diff --git a/packages/next/build/webpack-config.ts b/packages/next/build/webpack-config.ts index c6928595de9ca..57befd296aeab 100644 --- a/packages/next/build/webpack-config.ts +++ b/packages/next/build/webpack-config.ts @@ -547,7 +547,7 @@ export default async function getBaseWebpackConfig( useTypeScript && new ForkTsCheckerWebpackPlugin({ typescript: typeScriptPath, - async: false, + async: dev, useTypescriptIncrementalApi: true, checkSyntacticErrors: true, tsconfig: tsConfigPath, diff --git a/packages/next/build/webpack/plugins/fork-ts-checker-watcher-hook.ts b/packages/next/build/webpack/plugins/fork-ts-checker-watcher-hook.ts deleted file mode 100644 index f2822fc91cdae..0000000000000 --- a/packages/next/build/webpack/plugins/fork-ts-checker-watcher-hook.ts +++ /dev/null @@ -1,27 +0,0 @@ -import ForkTsCheckerWebpackPlugin from 'fork-ts-checker-webpack-plugin' -import { NormalizedMessage } from 'fork-ts-checker-webpack-plugin/lib/NormalizedMessage' -import webpack from 'webpack' - -export function Apply(compiler: webpack.Compiler) { - const hooks = ForkTsCheckerWebpackPlugin.getCompilerHooks(compiler) - - let additionalFiles: string[] = [] - - hooks.receive.tap('ForkTsCheckerWatcherHook', function( - diagnostics: NormalizedMessage[], - lints: NormalizedMessage[] - ) { - additionalFiles = [ - ...new Set([ - ...diagnostics.map(d => d.file!), - ...lints.map(l => l.file!), - ]), - ].filter(Boolean) - }) - - compiler.hooks.afterCompile.tap('ForkTsCheckerWatcherHook', function( - compilation - ) { - additionalFiles.forEach(file => compilation.fileDependencies.add(file)) - }) -} diff --git a/packages/next/server/hot-reloader.js b/packages/next/server/hot-reloader.js index a615e4e47a8ec..8513974704f28 100644 --- a/packages/next/server/hot-reloader.js +++ b/packages/next/server/hot-reloader.js @@ -14,11 +14,10 @@ import { import { NEXT_PROJECT_ROOT_DIST_CLIENT } from '../lib/constants' import { route } from 'next-server/dist/server/router' import { createPagesMapping, createEntrypoints } from '../build/entries' -import { watchCompiler } from '../build/output' +import { watchCompilers } from '../build/output' import { findPageFile } from './lib/find-page-file' import { recursiveDelete } from '../lib/recursive-delete' import { promisify } from 'util' -import * as ForkTsCheckerWatcherHook from '../build/webpack/plugins/fork-ts-checker-watcher-hook' import fs from 'fs' const access = promisify(fs.access) @@ -310,13 +309,14 @@ export default class HotReloader { } async prepareBuildTools (multiCompiler) { - watchCompiler(multiCompiler.compilers[0], multiCompiler.compilers[1]) - const tsConfigPath = join(this.dir, 'tsconfig.json') const useTypeScript = await fileExists(tsConfigPath) - if (useTypeScript) { - ForkTsCheckerWatcherHook.Apply(multiCompiler.compilers[0]) - } + + watchCompilers( + multiCompiler.compilers[0], + multiCompiler.compilers[1], + useTypeScript + ) // This plugin watches for changes to _document.js and notifies the client side that it should reload the page multiCompiler.compilers[1].hooks.done.tap( From d09946d48fa29d543d76f160ac9ac681c2788e62 Mon Sep 17 00:00:00 2001 From: Joe Haddad Date: Tue, 25 Jun 2019 17:05:08 -0400 Subject: [PATCH 2/4] Add TODO --- packages/next/server/hot-reloader.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/next/server/hot-reloader.js b/packages/next/server/hot-reloader.js index 8513974704f28..342b806ef3215 100644 --- a/packages/next/server/hot-reloader.js +++ b/packages/next/server/hot-reloader.js @@ -318,6 +318,9 @@ export default class HotReloader { useTypeScript ) + // TODO: on type error + // TODO: emit via this.send + // This plugin watches for changes to _document.js and notifies the client side that it should reload the page multiCompiler.compilers[1].hooks.done.tap( 'NextjsHotReloaderForServer', @@ -494,7 +497,7 @@ export default class HotReloader { return [] } - send (action, ...args) { + send = (action, ...args) => { this.webpackHotMiddleware.publish({ action, data: args }) } From 97cd440f80bf76aab150e56393f6e0ed5aa21465 Mon Sep 17 00:00:00 2001 From: Joe Haddad Date: Wed, 26 Jun 2019 11:24:20 -0400 Subject: [PATCH 3/4] Fix webpack invalidation --- packages/next/build/output/index.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/next/build/output/index.ts b/packages/next/build/output/index.ts index b093336830133..34ce854fdf538 100644 --- a/packages/next/build/output/index.ts +++ b/packages/next/build/output/index.ts @@ -215,13 +215,14 @@ export function watchCompilers( hasTypeChecking: boolean, onEvent: (status: WebpackStatus) => void ) { + let tsMessagesPromise: Promise | undefined + let tsMessagesResolver: (diagnostics: CompilerDiagnostics) => void + compiler.hooks.invalid.tap(`NextJsInvalid-${key}`, () => { + tsMessagesPromise = undefined onEvent({ loading: true }) }) - let tsMessagesPromise: Promise - let tsMessagesResolver: (diagnostics: CompilerDiagnostics) => void - if (hasTypeChecking) { const typescriptFormatter = createCodeframeFormatter({}) @@ -281,6 +282,9 @@ export function watchCompilers( return } + stats.compilation.errors.push(...(typeMessages.errors || [])) + stats.compilation.warnings.push(...(typeMessages.warnings || [])) + onEvent({ loading: false, typeChecking: false, From fc90de017cf83cbda69b46d0f6fd02a93ed11bd0 Mon Sep 17 00:00:00 2001 From: Joe Haddad Date: Wed, 26 Jun 2019 12:43:16 -0400 Subject: [PATCH 4/4] Show TypeScript error in dev client --- packages/next/build/output/index.ts | 11 ++++- .../dev/error-overlay/hot-dev-client.js | 45 +++++++++++++++---- packages/next/server/hot-reloader.js | 6 +-- 3 files changed, 49 insertions(+), 13 deletions(-) diff --git a/packages/next/build/output/index.ts b/packages/next/build/output/index.ts index 34ce854fdf538..6bb16c6d7b049 100644 --- a/packages/next/build/output/index.ts +++ b/packages/next/build/output/index.ts @@ -198,7 +198,8 @@ export function ampValidation( export function watchCompilers( client: any, server: any, - enableTypeCheckingOnClient: boolean + enableTypeCheckingOnClient: boolean, + onTypeChecked: (diagnostics: CompilerDiagnostics) => void ) { if (previousClient === client && previousServer === server) { return @@ -284,6 +285,14 @@ export function watchCompilers( stats.compilation.errors.push(...(typeMessages.errors || [])) stats.compilation.warnings.push(...(typeMessages.warnings || [])) + onTypeChecked({ + errors: stats.compilation.errors.length + ? stats.compilation.errors + : null, + warnings: stats.compilation.warnings.length + ? stats.compilation.warnings + : null, + }) onEvent({ loading: false, diff --git a/packages/next/client/dev/error-overlay/hot-dev-client.js b/packages/next/client/dev/error-overlay/hot-dev-client.js index fc40c762e7a81..65ae9574af98f 100644 --- a/packages/next/client/dev/error-overlay/hot-dev-client.js +++ b/packages/next/client/dev/error-overlay/hot-dev-client.js @@ -122,6 +122,7 @@ export default function connect (options) { var isFirstCompilation = true var mostRecentCompilationHash = null var hasCompileErrors = false +let deferredBuildError = null function clearOutdatedErrors () { // Clean up outdated compile errors, if any. @@ -130,6 +131,8 @@ function clearOutdatedErrors () { console.clear() } } + + deferredBuildError = null } // Successful compilation. @@ -141,9 +144,13 @@ function handleSuccess () { // Attempt to apply hot updates or reload. if (isHotUpdate) { tryApplyUpdates(function onHotUpdateSuccess () { - // Only dismiss it when we're sure it's a hot update. - // Otherwise it would flicker right before the reload. - ErrorOverlay.dismissBuildError() + if (deferredBuildError) { + deferredBuildError() + } else { + // Only dismiss it when we're sure it's a hot update. + // Otherwise it would flicker right before the reload. + ErrorOverlay.dismissBuildError() + } }) } } @@ -220,22 +227,44 @@ function processMessage (e) { handleAvailableHash(obj.hash) } - if (obj.warnings.length > 0) { - handleWarnings(obj.warnings) - } + const { errors, warnings } = obj + const hasErrors = Boolean(errors && errors.length) - if (obj.errors.length > 0) { + const hasWarnings = Boolean(warnings && warnings.length) + + if (hasErrors) { // When there is a compilation error coming from SSR we have to reload the page on next successful compile if (obj.action === 'sync') { hadRuntimeError = true } - handleErrors(obj.errors) + + handleErrors(errors) break + } else if (hasWarnings) { + handleWarnings(warnings) } handleSuccess() break } + case 'typeChecked': { + const [{ errors, warnings }] = obj.data + const hasErrors = Boolean(errors && errors.length) + + const hasWarnings = Boolean(warnings && warnings.length) + + if (hasErrors) { + if (canApplyUpdates()) { + handleErrors(errors) + } else { + deferredBuildError = () => handleErrors(errors) + } + } else if (hasWarnings) { + handleWarnings(warnings) + } + + break + } default: { if (customHmrEventHandler) { customHmrEventHandler(obj) diff --git a/packages/next/server/hot-reloader.js b/packages/next/server/hot-reloader.js index 342b806ef3215..8308b6e7efc28 100644 --- a/packages/next/server/hot-reloader.js +++ b/packages/next/server/hot-reloader.js @@ -315,12 +315,10 @@ export default class HotReloader { watchCompilers( multiCompiler.compilers[0], multiCompiler.compilers[1], - useTypeScript + useTypeScript, + ({ errors, warnings }) => this.send('typeChecked', { errors, warnings }) ) - // TODO: on type error - // TODO: emit via this.send - // This plugin watches for changes to _document.js and notifies the client side that it should reload the page multiCompiler.compilers[1].hooks.done.tap( 'NextjsHotReloaderForServer',