From 8da6d1e60d88ca54cb9d882ac3fbcc242829cd1d Mon Sep 17 00:00:00 2001
From: Calvin French-Owen <calvin@calv.info>
Date: Fri, 18 May 2018 17:33:17 -0700
Subject: [PATCH] .ready: fix behavior when packages fail to load
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Netto brought an error with the Appcues integration up to my
attention. Apparently when the script fails to load, it causes
an exception which bubbles up to the top.

I looked through the code and was pretty surprised by this, since
the Appcues integration itself is very few lines of code, and
nothing out of the ordinary.

When I dug in, I realized that this comes from an error where the
Appcues javascript fails to load for some reason.

When that happens here's the sequence of events:

- segmentio/load-script gets called, and then calls a callback
  with the error
- this in turn calls .ready (in Appcues, but I think also many
  other integrations)
- this emits a ready handler, which then immediately starts
  flushing the message queue, causing an uncaught exception

This fix instead causes .ready when called with an error to
_not_ mark the integration as ready. Avoiding further errors.

The one thing I'm unsure of here is whether we ever end up calling
.ready() with an argument–my hunch is 'no' given that it wouldn't
do anything, but it might be worth adding an instanceof Error check.

Thoughts?
---
 lib/protos.js      |  3 ++-
 test/index.test.js | 24 ++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/lib/protos.js b/lib/protos.js
index c339c47..3dad067 100644
--- a/lib/protos.js
+++ b/lib/protos.js
@@ -304,7 +304,8 @@ exports.locals = function(locals) {
  * @api public
  */
 
-exports.ready = function() {
+exports.ready = function(err) {
+  if (err) return this.debug('error loading "%s" error="%s"', this.name, err);
   this.emit('ready');
 };
 
diff --git a/test/index.test.js b/test/index.test.js
index 44d93d0..fd310ac 100644
--- a/test/index.test.js
+++ b/test/index.test.js
@@ -633,4 +633,28 @@ describe('integration', function() {
       assert(window.onload === onload);
     });
   });
+
+  describe('#ready', function() {
+    beforeEach(function() {
+      integration = new Integration();
+    });
+
+    it('should should emit a ready event', function() {
+      var called = false;
+      integration.on('ready', function() {
+        called = true;
+      });
+      integration.ready();
+      assert(called === true);
+    });
+
+    it('should should not emit a ready event if the integration fails', function() {
+      var called = false;
+      integration.on('ready', function() {
+        called = true;
+      });
+      integration.ready(new Error('failed to load'));
+      assert(called === false);
+    });
+  });
 });