Repository: mapbox/batfish
Pull request #307 (2019-01-30 20:00:50)

Remove build cruft from output directory
Addresses #300.

This is just a WIP PR (from pairing) that @danswick will finish up.

danswick: Next steps: 

- [ ] Document now-optional Webpack stats config.
- [ ] Remove `assets.json`, `static-render-pages.js`, and `static-render-pages.js.map` when build is complete.
- [ ] Update tests.
katydecorah: This week I learned that [Webpack 4 generates a much larger stats.json file](https://medium.com/coursera-engineering/webpack-3-to-4-facing-the-known-unknowns-and-unknown-unknowns-cdfeb817faf8#0de7) than Webpack 3. For example, traffic-data is a small site with 4 pages:
- Webpack 3, stats.json: `8.3 mb` 
- Webpack 4, stats.json: `17.7mb`

On Webpack 4, mapbox-gl-js-docs runs out of memory when attempting to build the site (locally and on staging). Using `node --inspect` showed me that node runs out of memory when trying to write stats.json. This must be a huge file considering we have 100+ example pages in that repo and tracks with the error message (re: `Very long string`):

```
Security context: 0x217004a25891 <JSObject>
    1: from [buffer.js:~175] [pc=0x1bd39f199701](this=0x21704e235dd9 <JSFunction Buffer (sfi = 0x217004a7e561)>,value=0x21703705a921 <Very long string[512278021]>,encodingOrOffset=0x217004a35b61 <String[4]: utf8>,length=0x2170fca822d1 <undefined>)
    2: arguments adaptor frame: 2->3
    3: writeFd(aka writeFd) [fs.js:1287] [bytecode=0x2170c28416b9 offset=62](this=0x2170fca822d1 <undefined>,fd=2...
```

I've tested this branch locally with mapbox-gl-js-docs and got the site to build with out running out of memory.

(Note: mapbox-gl-js-docs is still slower to build than repos of similar size. We'll continue to investigate improvements.)

@danswick other than bringing this PR up-to-date with master, do you recall anything else to be completed in this PR?
danswick: @katydecorah I think this PR is good to go. I think we addressed the relevant issues back in January. I also tested this branch locally and it seems to be working as expected! 

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 0d00b81..234ad63 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,10 @@
 # Changelog
 
+## Master
+
+- **Fix:** Prevent temporary build files from being written to the output directory.
+- **Feature:** Batfish no longer generates a stats.json at build time. Pass the `--stats` flag with the `build` command to generate stats.json.
+
 ## 1.10.2
 
 - **Fix:** Use core-js@2 and fix core-js modules so that they are required.
diff --git a/bin/batfish.js b/bin/batfish.js
index a317bd5..0ec9fab 100755
--- a/bin/batfish.js
+++ b/bin/batfish.js
@@ -55,6 +55,7 @@ ${chalk.bold(`${chalk.magenta('start')} options`)}
 ${chalk.bold(`${chalk.magenta('build')} options`)}
   -d, --debug      Build for debugging, not for production.
   --no-clear       Do not clear the destination directory.
+  -s, --stats      Generate Webpack statistics.
 
 ${chalk.bold(`${chalk.magenta('serve-static')} options`)}
   -p, --port       Server port. Default: 8080.
@@ -94,6 +95,10 @@ const cli = meow({
       type: 'boolean',
       alias: 'V'
     },
+    stats: {
+      type: 'boolean',
+      alias: 's'
+    },
     port: {
       type: 'number',
       alias: 'p'
@@ -178,6 +183,9 @@ if (cli.flags.port) {
 if (cli.flags.verbose) {
   config.verbose = cli.flags.verbose;
 }
+if (cli.flags.stats) {
+  config.webpackStats = true;
+}
 if (command === 'start' && cli.flags.include) {
   config.includePages = [].concat(cli.flags.include);
 }
diff --git a/docs/advanced-usage.md b/docs/advanced-usage.md
index ba70064..80ab7a0 100644
--- a/docs/advanced-usage.md
+++ b/docs/advanced-usage.md
@@ -259,7 +259,7 @@ To attach listeners to route change events (e.g. add a page-loading animation),
 
 ## Analyzing bundles
 
-Batfish's `start` and `end` commands output [Webpack's `stats.json`](https://webpack.js.org/api/stats/) so you can use it to analyze the composition of your bundles.
+Batfish's `--stats` flag, when used with the `build` command, will output [Webpack's `stats.json`](https://webpack.js.org/api/stats/) so you can use it to analyze the composition of your bundles.
 
 [webpack-bundle-analyzer](https://github.com/th0r/webpack-bundle-analyzer) and [webpack.github.io/analyse](https://webpack.github.io/analyse/) are two great tools that you can feed your `stats.json` to.
 There are also others out there in the Webpack ecosystem.
diff --git a/docs/cli.md b/docs/cli.md
index eeb86c7..19ebb14 100644
--- a/docs/cli.md
+++ b/docs/cli.md
@@ -37,6 +37,7 @@ start options
 build options
   -d, --debug      Build for debugging, not for production.
   --no-clear       Do not clear the destination directory.
+  -s, --stats      Generate Webpack statistics.
 
 serve-static options
   -p, --port       Server port. Default: 8080.
diff --git a/src/node/build.js b/src/node/build.js
index 40192a3..8b94f51 100644
--- a/src/node/build.js
+++ b/src/node/build.js
@@ -11,6 +11,7 @@ const appendTaskTime = require('./append-task-time');
 const inlineCss = require('./inline-css');
 const generateSitemap = require('./generate-sitemap');
 const compileStylesheets = require('./compile-stylesheets');
+const cleanUpTempFiles = require('./clean-up-temp-files');
 const constants = require('./constants');
 const maybeClearOutputDirectory = require('./maybe-clear-output-directory');
 const nonPageFiles = require('./non-page-files');
@@ -65,7 +66,9 @@ function build(rawConfig?: Object, projectDirectory?: string): EventEmitter {
             stats.endTime
           )
         );
-        return writeWebpackStats(outputDirectory, stats);
+        if (tailoredBatfishConfig.webpackStats) {
+          return writeWebpackStats(outputDirectory, stats);
+        }
       });
   };
 
@@ -157,6 +160,11 @@ function build(rawConfig?: Object, projectDirectory?: string): EventEmitter {
         return generateSitemap(batfishConfig);
       });
     })
+    .then(() => {
+      return logTask('cleaning up temporary files', () => {
+        return cleanUpTempFiles(batfishConfig);
+      });
+    })
     .then(() => {
       emitNotification(appendTaskTime('Finished build', buildStartTime));
       emitter.emit(constants.EVENT_DONE);
diff --git a/src/node/clean-up-temp-files.js b/src/node/clean-up-temp-files.js
new file mode 100644
index 0000000..e9a3377
--- /dev/null
+++ b/src/node/clean-up-temp-files.js
@@ -0,0 +1,22 @@
+// @flow
+'use strict';
+
+const path = require('path');
+const del = require('del');
+
+const cleanUpTempFiles = (
+  batfishConfig: BatfishConfiguration
+): Promise<any> => {
+  return del(
+    ['assets.json', 'static-render-pages.js', 'static-render-pages.js.map'],
+    {
+      force: true,
+      cwd: path.join(
+        batfishConfig.outputDirectory,
+        batfishConfig.publicAssetsPath
+      )
+    }
+  );
+};
+
+module.exports = cleanUpTempFiles;
diff --git a/src/node/validate-config.js b/src/node/validate-config.js
index 90b5a10..f9578fc 100644
--- a/src/node/validate-config.js
+++ b/src/node/validate-config.js
@@ -196,6 +196,10 @@ const configSchema = {
     validator: _.isBoolean,
     description: 'boolean'
   },
+  webpackStats: {
+    validator: _.isBoolean,
+    description: 'boolean'
+  },
   includePages: {
     validator: isArrayOf(_.isString),
     description: 'globs relative to the pagesDirectory, not absolute paths'
@@ -270,7 +274,8 @@ function validateConfig(
     clearOutputDirectory: true,
     spa: false,
     webpackStaticStubReactComponent: [],
-    sitemap: true
+    sitemap: true,
+    webpackStats: false
   };
 
   const config = Object.assign({}, defaults, rawConfig);
diff --git a/src/node/watch-webpack.js b/src/node/watch-webpack.js
index d7cd5c5..b12d370 100644
--- a/src/node/watch-webpack.js
+++ b/src/node/watch-webpack.js
@@ -14,7 +14,6 @@ const constants = require('./constants');
 const createWebpackConfigClient = require('./create-webpack-config-client');
 const createWebpackStatsError = require('./create-webpack-stats-error');
 const watchContext = require('./watch-context');
-const writeWebpackStats = require('./write-webpack-stats');
 
 function watchWebpack(
   batfishConfig: BatfishConfiguration,
@@ -77,7 +76,6 @@ function watchWebpack(
           onError(createWebpackStatsError(stats));
         }
 
-        writeWebpackStats(batfishConfig.outputDirectory, stats).catch(onError);
         if (batfishConfig.verbose) {
           onNotification(
             stats.toString({


Test patch:
diff --git a/test/__snapshots__/build.test.js.snap b/test/__snapshots__/build.test.js.snap
index 009c075..2d42d8e 100644
--- a/test/__snapshots__/build.test.js.snap
+++ b/test/__snapshots__/build.test.js.snap
@@ -18,6 +18,9 @@ Array [
   "Finished inlining CSS in static HTML in 0ms",
   "Building sitemap ...",
   "Finished building sitemap in 0ms",
+  "Cleaning up temporary files ...",
+  "Starting build ...",
+  "Finished cleaning up temporary files in 0ms",
   "Finished build in 0ms",
 ]
 `;
@@ -32,6 +35,9 @@ Array [
   "Finished copying non-page files in 0ms",
   "Writing static html ...",
   "Finished writing static HTML in 0ms",
+  "Cleaning up temporary files ...",
+  "Starting build ...",
+  "Finished cleaning up temporary files in 0ms",
   "Finished build in 0ms",
 ]
 `;
diff --git a/test/__snapshots__/validate-config.test.js.snap b/test/__snapshots__/validate-config.test.js.snap
index 184abd5..b1c3d06 100644
--- a/test/__snapshots__/validate-config.test.js.snap
+++ b/test/__snapshots__/validate-config.test.js.snap
@@ -53,5 +53,6 @@ Object {
   "temporaryDirectory": "/my-project/_batfish_tmp",
   "verbose": false,
   "webpackStaticStubReactComponent": Array [],
+  "webpackStats": false,
 }
 `;
diff --git a/test/build.test.js b/test/build.test.js
index e2dee3e..d026300 100644
--- a/test/build.test.js
+++ b/test/build.test.js
@@ -81,7 +81,8 @@ describe('build', () => {
       siteOrigin: 'https://www.mapbox.com',
       verbose: false,
       staticHtmlInlineDeferCss: true,
-      sitemap: true
+      sitemap: true,
+      webpackStats: true
     };
   });
 
diff --git a/test/watch-webpack.test.js b/test/watch-webpack.test.js
index 6600f89..efd64fd 100644
--- a/test/watch-webpack.test.js
+++ b/test/watch-webpack.test.js
@@ -5,7 +5,6 @@ const watchWebpack = require('../src/node/watch-webpack');
 const createWebpackConfigClient = require('../src/node/create-webpack-config-client');
 const errorTypes = require('../src/node/error-types');
 const watchContext = require('../src/node/watch-context');
-const writeWebpackStats = require('../src/node/write-webpack-stats');
 
 jest.mock('webpack', () => {
   const compiler = {
@@ -145,58 +144,4 @@ describe('watchWebpack', () => {
       done();
     });
   });
-
-  test('handles errors in stats', (done) => {
-    const erroneousStats = createMockStats('a');
-    erroneousStats.hasErrors.mockReturnValue(true);
-    watchWebpack(batfishConfig, { onError, onNotification, onFirstCompile });
-    expect.hasAssertions();
-    process.nextTick(() => {
-      // Invoke the watcher callback with an error.
-      webpack.compiler.watch.mock.calls[0][1](null, erroneousStats);
-      expect(onError).toHaveBeenCalled();
-      const error = onError.mock.calls[0][0];
-      expect(error).toBeInstanceOf(errorTypes.WebpackCompilationError);
-      expect(error.stats).toBe(erroneousStats);
-      done();
-    });
-  });
-
-  test('on every new compilation, writes stats', (done) => {
-    watchWebpack(batfishConfig, { onError, onNotification, onFirstCompile });
-    process.nextTick(() => {
-      const firstStats = createMockStats('a');
-      webpack.compiler.watch.mock.calls[0][1](null, firstStats);
-      expect(writeWebpackStats).toHaveBeenCalledTimes(1);
-      expect(writeWebpackStats).toHaveBeenCalledWith(
-        batfishConfig.outputDirectory,
-        firstStats
-      );
-      const secondStats = createMockStats('b');
-      webpack.compiler.watch.mock.calls[0][1](null, secondStats);
-      expect(writeWebpackStats).toHaveBeenCalledTimes(2);
-      expect(writeWebpackStats).toHaveBeenCalledWith(
-        batfishConfig.outputDirectory,
-        secondStats
-      );
-      done();
-    });
-  });
-
-  test('handles errors writing stats', () => {
-    const expectedError = new Error();
-    const statsResult = Promise.reject(expectedError);
-    writeWebpackStats.mockReturnValueOnce(statsResult);
-    watchWebpack(batfishConfig, { onError, onNotification, onFirstCompile });
-    return statsResult
-      .catch(() => {
-        webpack.compiler.watch.mock.calls[0][1](null, createMockStats('a'));
-        expect(writeWebpackStats).toHaveBeenCalledTimes(1);
-      })
-      .then(() => {
-        expect(onError).toHaveBeenCalled();
-        const error = onError.mock.calls[0][0];
-        expect(error).toBe(expectedError);
-      });
-  });
 });