FIX issue where webpack process would hang when manager build failed

Also cleaned up a little making the whole code path simpler

lib/core/src/server/build-dev.js

@@ -5,7 +5,7 @@ import favicon from 'serve-favicon';
 import path from 'path';
 import fs from 'fs-extra';
 import chalk from 'chalk';
-import { logger, colors } from '@storybook/node-logger';
+import { logger, colors, instance as npmLog } from '@storybook/node-logger';
 import fetch from 'node-fetch';
 import Cache from 'file-system-cache';
 import findCacheDir from 'find-cache-dir';
@@ -16,7 +16,7 @@ import { stripIndents } from 'common-tags';
 import Table from 'cli-table3';
 import prettyTime from 'pretty-hrtime';
 
-import storybook, { webpackValid } from './dev-server';
+import storybook from './dev-server';
 import { getDevCli } from './cli';
 
 const defaultFavIcon = require.resolve('./public/favicon.ico');
@@ -225,10 +225,10 @@ async function outputStats(previewStats, managerStats) {
 function openInBrowser(address) {
   opn(address).catch(() => {
     logger.error(stripIndents`
-          Could not open ${address} inside a browser. If you're running this command inside a
-          docker container or on a CI, you need to pass the '--ci' flag to prevent opening a
-          browser by default.
-        `);
+      Could not open ${address} inside a browser. If you're running this command inside a
+      docker container or on a CI, you need to pass the '--ci' flag to prevent opening a
+      browser by default.
+    `);
   });
 }
 
@@ -248,17 +248,20 @@ export async function buildDevStandalone(options) {
 
     await applyStatic(app, options);
 
-    const storybookMiddleware = await storybook(options);
+    const {
+      router: storybookMiddleware,
+      previewStats,
+      managerStats,
+      managerTotalTime,
+      previewTotalTime,
+    } = await storybook(options);
 
     app.use(storybookMiddleware);
 
     const serverListening = listenToServer(server, listenAddr);
     const { version } = options.packageJson;
 
-    const [
-      { previewStats, managerStats, managerTotalTime, previewTotalTime },
-      updateInfo,
-    ] = await Promise.all([webpackValid, updateCheck(version), serverListening]);
+    const [updateInfo] = await Promise.all([updateCheck(version), serverListening]);
 
     const proto = options.https ? 'https' : 'http';
     const address = `${proto}://${options.host || 'localhost'}:${port}/`;
@@ -282,8 +285,34 @@ export async function buildDevStandalone(options) {
       openInBrowser(address);
     }
   } catch (error) {
+    // this is a weird bugfix, somehow 'node-pre-gyp' is poluting the npmLog header
+    npmLog.heading = '';
+
+    logger.line();
+    logger.warn(
+      error.close
+        ? stripIndents`
+            FATAL broken build!, will close the process,
+            Fix the error below and restart storybook.
+          `
+        : stripIndents`
+            Broken build, fix the error below.
+            You may need to refresh the browser.
+          `
+    );
+    logger.line();
     if (error instanceof Error) {
-      logger.error(error);
+      if (error.error) {
+        logger.error(error.error);
+      } else if (error.stats && error.stats.compilation.errors) {
+        error.stats.compilation.errors.forEach(e => logger.plain(e));
+      } else {
+        logger.error(Object.keys(error.stats.compilation));
+      }
+
+      if (error.close) {
+        process.exit(1);
+      }
     }
     if (options.smokeTest) {
       process.exit(1);

lib/core/src/server/dev-server.js

@@ -5,24 +5,22 @@ import webpack from 'webpack';
 import webpackDevMiddleware from 'webpack-dev-middleware';
 import webpackHotMiddleware from 'webpack-hot-middleware';
 
+import { logger } from '@storybook/node-logger';
 import { getMiddleware } from './utils/middleware';
 
 import loadConfig from './config';
 import loadManagerConfig from './manager/manager-config';
 
-let webpackResolve = () => {};
-let webpackReject = () => {};
-
 const dllPath = path.join(__dirname, '../../dll');
 
-export const webpackValid = new Promise((resolve, reject) => {
-  webpackResolve = resolve;
-  webpackReject = reject;
-});
-
 const cache = {};
 
-export default async function(options) {
+let previewProcess;
+let previewReject;
+
+const router = new Router();
+
+export default function(options) {
   const configDir = path.resolve(options.configDir);
   const outputDir = path.resolve(options.outputDir || path.join(__dirname, '..', 'public'));
   const configType = 'DEVELOPMENT';
@@ -48,10 +46,22 @@ export default async function(options) {
           },
           (err, stats) => {
             managerTotalTime = process.hrtime(startTime);
-            if (err) {
-              reject(err);
-            } else if (stats.hasErrors()) {
-              reject(stats);
+            if (err || stats.hasErrors()) {
+              const error = new Error('Manager build is broken');
+              error.error = err;
+              error.close = true;
+              error.stats = stats;
+              logger.line();
+              logger.line();
+              try {
+                previewReject(error);
+                previewProcess.close();
+                logger.warn('force closed preview build');
+              } catch (e) {
+                logger.warn('Unable to close preview build!');
+              }
+              logger.line();
+              reject(error);
             } else {
               resolve(stats);
             }
@@ -60,82 +70,82 @@ export default async function(options) {
       })
   );
 
-  const iframeConfig = await loadConfig({
+  const previewPromise = loadConfig({
     configType,
     outputDir,
     cache,
     corePresets: [require.resolve('./preview/preview-preset.js')],
     overridePresets: [require.resolve('./preview/custom-webpack-preset.js')],
     ...options,
-  });
+  }).then(previewConfig => {
+    const middlewareFn = getMiddleware(configDir);
 
-  const middlewareFn = getMiddleware(configDir);
+    // remove the leading '/'
+    let { publicPath } = previewConfig.output;
+    if (publicPath[0] === '/') {
+      publicPath = publicPath.slice(1);
+    }
 
-  // remove the leading '/'
-  let { publicPath } = iframeConfig.output;
-  if (publicPath[0] === '/') {
-    publicPath = publicPath.slice(1);
-  }
+    const previewCompiler = webpack(previewConfig);
+    const devMiddlewareOptions = {
+      publicPath: previewConfig.output.publicPath,
+      watchOptions: {
+        aggregateTimeout: 1,
+        ignored: /node_modules/,
+        ...(previewConfig.watchOptions || {}),
+      },
+      // this actually causes 0 (regular) output from wdm & webpack
+      logLevel: 'warn',
+      clientLogLevel: 'warning',
+      noInfo: true,
+      ...previewConfig.devServer,
+    };
 
-  const iframeCompiler = webpack(iframeConfig);
-  const devMiddlewareOptions = {
-    publicPath: iframeConfig.output.publicPath,
-    watchOptions: {
-      aggregateTimeout: 1,
-      ignored: /node_modules/,
-      ...(iframeConfig.watchOptions || {}),
-    },
-    // this actually causes 0 (regular) output from wdm & webpack
-    logLevel: 'warn',
-    clientLogLevel: 'warning',
-    noInfo: true,
-    ...iframeConfig.devServer,
-  };
+    const webpackDevMiddlewareInstance = webpackDevMiddleware(
+      previewCompiler,
+      devMiddlewareOptions
+    );
+    router.use(webpackDevMiddlewareInstance);
+    router.use(webpackHotMiddleware(previewCompiler));
 
-  const router = new Router();
-  const webpackDevMiddlewareInstance = webpackDevMiddleware(iframeCompiler, devMiddlewareOptions);
-  router.use(webpackDevMiddlewareInstance);
-  router.use(webpackHotMiddleware(iframeCompiler));
+    // custom middleware
+    middlewareFn(router);
 
-  // custom middleware
-  middlewareFn(router);
+    return new Promise((resolve, reject) => {
+      previewReject = reject;
+      webpackDevMiddlewareInstance.waitUntilValid(stats => {
+        previewTotalTime = process.hrtime(startTime);
 
-  const previewPromise = new Promise((resolve, reject) => {
-    webpackDevMiddlewareInstance.waitUntilValid(stats => {
-      previewTotalTime = process.hrtime(startTime);
-
-      if (!stats) {
-        reject(new Error('no stats after building iframe'));
-      } else if (stats.hasErrors()) {
-        reject(stats);
-      } else {
-        resolve(stats);
-      }
+        if (!stats) {
+          reject(new Error('no stats after building preview'));
+        } else if (stats.hasErrors()) {
+          reject(stats);
+        } else {
+          resolve(stats);
+        }
+      });
+      previewProcess = webpackDevMiddlewareInstance;
     });
   });
 
-  Promise.all([managerPromise, previewPromise])
-    .then(([managerStats, previewStats]) => {
-      router.get('/', (request, response) => {
-        response.set('Content-Type', 'text/html');
-        response.sendFile(path.join(`${outputDir}/index.html`));
-      });
-      router.get(/\/sb_dll\/(.+\.js)$/, (request, response) => {
-        response.set('Content-Type', 'text/javascript');
-        response.sendFile(path.join(`${dllPath}/${request.params[0]}`));
-      });
-      router.get(/\/sb_dll\/(.+\.LICENCE)$/, (request, response) => {
-        response.set('Content-Type', 'text/html');
-        response.sendFile(path.join(`${dllPath}/${request.params[0]}`));
-      });
-      router.get(/(.+\.js)$/, (request, response) => {
-        response.set('Content-Type', 'text/javascript');
-        response.sendFile(path.join(`${outputDir}/${request.params[0]}`));
-      });
+  return Promise.all([managerPromise, previewPromise]).then(([managerStats, previewStats]) => {
+    router.get('/', (request, response) => {
+      response.set('Content-Type', 'text/html');
+      response.sendFile(path.join(`${outputDir}/index.html`));
+    });
+    router.get(/\/sb_dll\/(.+\.js)$/, (request, response) => {
+      response.set('Content-Type', 'text/javascript');
+      response.sendFile(path.join(`${dllPath}/${request.params[0]}`));
+    });
+    router.get(/\/sb_dll\/(.+\.LICENCE)$/, (request, response) => {
+      response.set('Content-Type', 'text/html');
+      response.sendFile(path.join(`${dllPath}/${request.params[0]}`));
+    });
+    router.get(/(.+\.js)$/, (request, response) => {
+      response.set('Content-Type', 'text/javascript');
+      response.sendFile(path.join(`${outputDir}/${request.params[0]}`));
+    });
 
-      webpackResolve({ previewStats, managerStats, managerTotalTime, previewTotalTime });
-    })
-    .catch(e => webpackReject(e));
-
-  return router;
+    return { previewStats, managerStats, managerTotalTime, previewTotalTime, router };
+  });
 }

lib/node-logger/src/index.ts

@@ -14,8 +14,12 @@ export const colors = {
 
 export const logger = {
   info: (message: string): void => npmLog.info('', message),
+  plain: (message: string): void => console.log(message),
+  line: (count: number = 1): void => console.log(`${Array(count - 1).fill('\n')}`),
   warn: (message: string): void => npmLog.warn('', message),
   error: (message: string): void => npmLog.error('', message),
   trace: ({ message, time }: { message: string; time: [number, number] }): void =>
     npmLog.info('', `${message} (${colors.purple(prettyTime(time))})`),
 };
+
+export { npmLog as instance };