Skip to content

Commit 8da6d1e

Browse files
committed
.ready: fix behavior when packages fail to load
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?
1 parent 9392b72 commit 8da6d1e

File tree

2 files changed

+26
-1
lines changed

2 files changed

+26
-1
lines changed

lib/protos.js

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -304,7 +304,8 @@ exports.locals = function(locals) {
304304
* @api public
305305
*/
306306

307-
exports.ready = function() {
307+
exports.ready = function(err) {
308+
if (err) return this.debug('error loading "%s" error="%s"', this.name, err);
308309
this.emit('ready');
309310
};
310311

test/index.test.js

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -633,4 +633,28 @@ describe('integration', function() {
633633
assert(window.onload === onload);
634634
});
635635
});
636+
637+
describe('#ready', function() {
638+
beforeEach(function() {
639+
integration = new Integration();
640+
});
641+
642+
it('should should emit a ready event', function() {
643+
var called = false;
644+
integration.on('ready', function() {
645+
called = true;
646+
});
647+
integration.ready();
648+
assert(called === true);
649+
});
650+
651+
it('should should not emit a ready event if the integration fails', function() {
652+
var called = false;
653+
integration.on('ready', function() {
654+
called = true;
655+
});
656+
integration.ready(new Error('failed to load'));
657+
assert(called === false);
658+
});
659+
});
636660
});

0 commit comments

Comments
 (0)