🐛 Fixed following redirects for images (#101)

closes #99 - Used [`got`](https://github.com/sindresorhus/got) for image-size requests, which is more lightweight and has the huge advantage of following redirects - Added and updated test - indents - decreased timeout to 3s instead of 5s - Added dependencies: - [validator](https://github.com/chriso/validator.js) - [lodash](https://github.com/lodash/lodash) - [got](https://github.com/sindresorhus/got) - Removed unused error conditionals
jbhannah · Oct 3, 2017 · b37c94a · b37c94a
1 parent 65d7037
commit b37c94a
Show file tree

Hide file tree

Showing 3 changed files with 230 additions and 106 deletions.
diff --git a/lib/amperize.js b/lib/amperize.js
@@ -1,16 +1,16 @@
 'use strict';
 
-var merge = require('lodash.merge')
-  , EventEmitter = require('events').EventEmitter
+var EventEmitter = require('events').EventEmitter
   , emits = require('emits')
   , html = require('htmlparser2')
   , util = require('util')
   , uuid = require('uuid')
   , async = require('async')
   , url = require('url')
-  , http = require('http')
-  , https = require('https')
+  , got = require('got')
+  , _ = require('lodash')
   , sizeOf = require('image-size')
+  , validator = require('validator')
   , helpers = require('./helpers');
 
 var DEFAULTS = {
@@ -32,8 +32,6 @@ var DEFAULTS = {
   }
 };
 
-var called;
-
 /**
  * Amperizer constructor. Borrows from Minimize.
  *
@@ -44,7 +42,7 @@ var called;
  * @api public
  */
 function Amperize(options) {
-  this.config = merge({}, DEFAULTS, options || {});
+  this.config = _.merge({}, DEFAULTS, options || {});
   this.emits = emits;
 
   this.htmlParser = new html.Parser(
@@ -105,19 +103,11 @@ Amperize.prototype.traverse = function traverse(data, html, done) {
     var children;
 
     function close(error, html) {
-      if (error) {
-        return step(error);
-      }
-
       html += helpers.close(element);
       step(null, html);
     }
 
     function enter(error) {
-      if (error) {
-        return step(error);
-      }
-
       children = element.children;
       html += helpers[element.type](element);
 
@@ -129,21 +119,21 @@ Amperize.prototype.traverse = function traverse(data, html, done) {
     }
 
     function useSecureSchema(element) {
-        if (element.attribs && element.attribs.src) {
-            // Every src attribute must be with 'https' protocol otherwise it will not get validated by AMP.
-            // If we're unable to replace it, we will deal with the valitation error, but at least
-            // we tried.
-            if (element.attribs.src.indexOf('https://') === -1) {
-                if (element.attribs.src.indexOf('http://') === 0) {
-                    // Replace 'http' with 'https', so the validation passes
-                    element.attribs.src = element.attribs.src.replace(/^http:\/\//i, 'https://');
-                } else if (element.attribs.src.indexOf('//') === 0) {
-                    // Giphy embedded iFrames are without protocol and start with '//', so at least
-                    // we can fix those cases.
-                    element.attribs.src = 'https:' + element.attribs.src;
-                }
-            }
+      if (element.attribs && element.attribs.src) {
+        // Every src attribute must be with 'https' protocol otherwise it will not get validated by AMP.
+        // If we're unable to replace it, we will deal with the valitation error, but at least
+        // we tried.
+        if (element.attribs.src.indexOf('https://') === -1) {
+          if (element.attribs.src.indexOf('http://') === 0) {
+            // Replace 'http' with 'https', so the validation passes
+            element.attribs.src = element.attribs.src.replace(/^http:\/\//i, 'https://');
+          } else if (element.attribs.src.indexOf('//') === 0) {
+            // Giphy embedded iFrames are without protocol and start with '//', so at least
+            // we can fix those cases.
+            element.attribs.src = 'https:' + element.attribs.src;
+          }
         }
+      }
 
       return;
     }
@@ -170,49 +160,52 @@ Amperize.prototype.traverse = function traverse(data, html, done) {
      * @return {Object} element incl. width and height
      */
     function getImageSize(element) {
-      var options = url.parse(element.attribs.src),
-          timeout = 5000,
-          request = element.attribs.src.indexOf('https') === 0 ? https : http;
+      var imageObj = url.parse(element.attribs.src),
+          requestOptions,
+          timeout = 3000;
 
-      called = false;
+      if (!validator.isURL(imageObj.href)) {
+        // revert this element, do not show
+        element.name = 'img';
 
-      // We need the user-agent, otherwise some https request may fail (e. g. cloudfare)
-      options.headers = { 'User-Agent': 'Mozilla/5.0' };
-
-      return request.get(options, function (response) {
-        var chunks = [];
-        response.on('data', function (chunk) {
-          chunks.push(chunk);
-        }).on('end', function () {
-            try {
-                var dimensions = sizeOf(Buffer.concat(chunks));
-                element.attribs.width = dimensions.width;
-                element.attribs.height = dimensions.height;
-
-                return getLayoutAttribute(element);
-            } catch (err) {
-                if (called) return;
-                called = true;
-
-                // revert this element, do not show
-                element.name = 'img';
-                return enter();
-            }
-        });
-      }).on('socket', function (socket) {
-        socket.setTimeout(timeout);
-        socket.on('timeout', function () {
-            if (called) return;
-            called = true;
-
-            // revert this element, do not show
-            element.name = 'img';
-            return enter();
-        });
-      }).on('error', function () {
-        if (called) return;
-        called = true;
+        return enter();
+      }
 
+      // We need the user-agent, otherwise some https request may fail (e. g. cloudfare)
+      requestOptions = {
+        headers: {
+            'User-Agent': 'Mozilla/5.0'
+        },
+        timeout: timeout,
+        encoding: null
+      };
+
+      return got (
+        imageObj.href,
+        requestOptions
+      ).then(function (response) {
+        try {
+          // Using the Buffer rather than an URL requires to use sizeOf synchronously.
+          // See https://github.com/image-size/image-size#asynchronous
+          var dimensions = sizeOf(response.body);
+
+          // CASE: `.ico` files might have multiple images and therefore multiple sizes.
+          // We return the largest size found (image-size default is the first size found)
+          if (dimensions.images) {
+            dimensions.width = _.maxBy(dimensions.images, function (w) {return w.width;}).width;
+            dimensions.height = _.maxBy(dimensions.images, function (h) {return h.height;}).height;
+          }
+
+          element.attribs.width = dimensions.width;
+          element.attribs.height = dimensions.height;
+
+          return getLayoutAttribute(element);
+        } catch (err) {
+          // revert this element, do not show
+          element.name = 'img';
+          return enter();
+        }
+      }).catch(function (err) {
         // revert this element, do not show
         element.name = 'img';
         return enter();
@@ -229,7 +222,7 @@ Amperize.prototype.traverse = function traverse(data, html, done) {
 
       if (!element.attribs.width || !element.attribs.height || !element.attribs.layout) {
         if (element.attribs.src.indexOf('http') === 0) {
-            return getImageSize(element);
+          return getImageSize(element);
         }
       }
       // Fallback to default values for a local image
@@ -254,7 +247,7 @@ Amperize.prototype.traverse = function traverse(data, html, done) {
     }
 
     if (element.name === 'audio') {
-        element.name = 'amp-audio';
+      element.name = 'amp-audio';
     }
 
     useSecureSchema(element);

diff --git a/package.json b/package.json
@@ -25,23 +25,25 @@
   },
   "homepage": "https://github.com/jbhannah/amperize#readme",
   "dependencies": {
-    "async": "2.1.4",
-    "emits": "3.0.0",
-    "htmlparser2": "3.9.2",
+    "async": "^2.1.4",
+    "emits": "^3.0.0",
+    "got": "^7.1.0",
+    "htmlparser2": "^3.9.2",
     "image-size": "0.5.1",
-    "lodash.merge": "4.6.0",
+    "lodash": "^4.17.4",
     "nock": "^9.0.2",
     "rewire": "^2.5.2",
-    "uuid": "^3.0.0"
+    "uuid": "^3.0.0",
+    "validator": "^8.2.0"
   },
   "devDependencies": {
-    "chai": "3.5.0",
+    "chai": "^3.5.0",
     "cz-conventional-changelog": "1.2.0",
-    "istanbul": "0.4.5",
-    "mocha": "3.2.0",
+    "istanbul": "^0.4.5",
+    "mocha": "^3.2.0",
     "semantic-release": "6.3.2",
-    "sinon": "1.17.7",
-    "sinon-chai": "2.8.0"
+    "sinon": "^1.17.7",
+    "sinon-chai": "^2.8.0"
   },
   "config": {
     "commitizen": {