Simplify storagemanager (#1799)

* Remove unused gzip parameter for writing html.

* Simplify storagemanager.

* Simplify how promises are handled, and directories created.
* Move out gzip handling to the plugins that use it, so storagemanager just writes strings and buffers unmodified.
* Make argument order consistent for write methods.

lib/core/resultsStorage/storageManager.js

@@ -3,29 +3,17 @@
 const fs = require('fs-extra');
 const Promise = require('bluebird');
 const path = require('path');
-const zlib = require('zlib');
 
 const pathToFolder = require('./pathToFolder');
 
-Promise.promisifyAll(zlib);
-
 const mkdirp = Promise.promisify(require('mkdirp'));
 
-function write(dirPath, filename, data, gzip) {
-  return Promise.join(dirPath, filename, data, (dirPath, filename, data) => {
-    if (gzip) {
-      const buff = new Buffer(data, 'utf8');
-      return zlib
-        .gzipAsync(buff, {
-          level: 1
-        })
-        .then(buffer =>
-          fs.writeFile(path.join(dirPath, filename + '.gz'), buffer)
-        );
-    } else {
-      return fs.writeFile(path.join(dirPath, filename), data, 'utf8');
-    }
-  });
+function write(dirPath, filename, data) {
+  return fs.writeFile(path.join(dirPath, filename), data);
+}
+
+function isValidDirectoryName(name) {
+  return name !== undefined && name !== '';
 }
 
 module.exports = function storageManager(baseDir, storagePathPrefix) {
@@ -33,23 +21,24 @@ module.exports = function storageManager(baseDir, storagePathPrefix) {
     rootPathFromUrl(url) {
       return pathToFolder(url)
         .split('/')
-        .filter(Boolean)
+        .filter(isValidDirectoryName)
         .map(() => '..')
         .join('/')
         .concat('/');
     },
-    createDataDir(subDir) {
-      const pathSegments = [baseDir, subDir].filter(Boolean);
+    createDirectory(...subDirs) {
+      const pathSegments = [baseDir, ...subDirs].filter(isValidDirectoryName);
 
-      return Promise.resolve(path.join.apply(null, pathSegments)).tap(dirPath =>
-        mkdirp(dirPath)
+      const dirPath = path.join.apply(null, pathSegments);
+      return mkdirp(dirPath).then(() => dirPath);
+    },
+    writeData(data, filename) {
+      return this.createDirectory('data').then(dir =>
+        write(dir, filename, data)
       );
     },
-    writeData(filename, data) {
-      return write(this.createDataDir('data'), filename, data);
-    },
-    writeHtml(filename, data) {
-      return write(this.createDataDir(), filename, data);
+    writeHtml(html, filename) {
+      return this.createDirectory().then(dir => write(dir, filename, html));
     },
     getBaseDir() {
       return baseDir;
@@ -58,23 +47,18 @@ module.exports = function storageManager(baseDir, storagePathPrefix) {
       return storagePathPrefix;
     },
     copyToResultDir(filename) {
-      return Promise.join(this.createDataDir(), filename, (dirPath, filename) =>
-        fs.copy(filename, dirPath)
-      );
+      return this.createDirectory().then(dir => fs.copy(filename, dir));
     },
     createDirForUrl(url, subDir) {
-      const pathSegments = [baseDir, pathToFolder(url), subDir].filter(Boolean);
-
-      return Promise.resolve(path.join.apply(null, pathSegments)).tap(dirPath =>
-        mkdirp(dirPath)
+      return this.createDirectory(pathToFolder(url), subDir);
+    },
+    writeDataForUrl(data, filename, url, subDir) {
+      return this.createDirectory(pathToFolder(url), 'data', subDir).then(dir =>
+        write(dir, filename, data)
       );
     },
-    writeDataForUrl(data, filename, url, subDir, gzip) {
-      const dirPath = ['data', subDir].filter(Boolean).join(path.sep);
-      return write(this.createDirForUrl(url, dirPath), filename, data, gzip);
-    },
-    writeHtmlForUrl(html, filename, url, gzip) {
-      return write(this.createDirForUrl(url), filename, html, gzip);
+    writeHtmlForUrl(html, filename, url) {
+      return this.createDirForUrl(url).then(dir => write(dir, filename, html));
     }
   };
 };

lib/plugins/analysisstorer/index.js

@@ -44,7 +44,7 @@ module.exports = {
         fileName = message.type + '-' + message.group + '.json';
       }
 
-      return this.storageManager.writeData(fileName, jsonData);
+      return this.storageManager.writeData(jsonData, fileName);
     }
   }
 };

lib/plugins/harstorer/index.js

@@ -1,5 +1,10 @@
 'use strict';
 
+const Promise = require('bluebird');
+const zlib = require('zlib');
+
+Promise.promisifyAll(zlib);
+
 module.exports = {
   open(context, options) {
     this.storageManager = context.storageManager;
@@ -9,18 +14,21 @@ module.exports = {
     switch (message.type) {
       case 'browsertime.har':
       case 'webpagetest.har': {
-        const jsonData = JSON.stringify(message.data);
+        const json = JSON.stringify(message.data);
+
         if (this.gzipHAR) {
-          return this.storageManager.writeDataForUrl(
-            jsonData,
-            message.type,
-            message.url,
-            '',
-            true
-          );
+          return zlib
+            .gzipAsync(Buffer.from(json), { level: 1 })
+            .then(gziped =>
+              this.storageManager.writeDataForUrl(
+                gziped,
+                `${message.type}.gz`,
+                message.url
+              )
+            );
         } else {
           return this.storageManager.writeDataForUrl(
-            jsonData,
+            json,
             message.type,
             message.url
           );

lib/plugins/html/htmlBuilder.js

@@ -338,8 +338,8 @@ class HTMLBuilder {
     );
 
     return this.storageManager.writeHtml(
-      name + '.html',
-      renderer.renderTemplate(name, locals)
+      renderer.renderTemplate(name, locals),
+      name + '.html'
     );
   }
 }

lib/plugins/metrics/index.js

@@ -74,8 +74,8 @@ module.exports = {
     if (message.type === 'sitespeedio.render') {
       if (this.options.list) {
         this.storageManager.writeData(
-          'metrics.txt',
-          Object.keys(this.metrics).join('\n')
+          Object.keys(this.metrics).join('\n'),
+          'metrics.txt'
         );
       }
       if (this.options.filterList) {
@@ -86,7 +86,7 @@ module.exports = {
             output += type + '.' + filters + '\n';
           }
         }
-        return this.storageManager.writeData('configuredMetrics.txt', output);
+        return this.storageManager.writeData(output, 'configuredMetrics.txt');
       }
     }
   },

lib/plugins/tracestorer/index.js

@@ -1,5 +1,10 @@
 'use strict';
 
+const zlib = require('zlib');
+const Promise = require('bluebird');
+
+Promise.promisifyAll(zlib);
+
 module.exports = {
   open(context) {
     this.storageManager = context.storageManager;
@@ -8,13 +13,17 @@ module.exports = {
     switch (message.type) {
       case 'browsertime.chrometrace':
       case 'webpagetest.chrometrace': {
-        return this.storageManager.writeDataForUrl(
-          JSON.stringify(message.data, null, 0),
-          message.name,
-          message.url,
-          '',
-          true
-        );
+        const json = JSON.stringify(message.data);
+
+        return zlib
+          .gzipAsync(Buffer.from(json), { level: 1 })
+          .then(gziped =>
+            this.storageManager.writeDataForUrl(
+              gziped,
+              `${message.name}.gz`,
+              message.url
+            )
+          );
       }
     }
   }

lib/sitespeed.js

@@ -55,7 +55,7 @@ module.exports = {
     );
 
     return storageManager
-      .createDataDir('logs')
+      .createDirectory('logs')
       .then(logDir => {
         logging.configure(options, logDir);
       })