.ready: fix behavior when packages fail to load

[calvinfo]

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?

Relevant Appcues code: https://github.com/segment-integrations/analytics.js-integration-appcues/blob/master/lib/index.js#L27-L29

Relevant a.js integration load code: https://github.com/segmentio/analytics.js-integration/blob/master/lib/protos.js#L237-L280

[calvinfo]

cc @nettofarah this should fix the uncaughts we're seeing

[calvinfo]

Separately, we should probably think about a way to make sure that this is try-catched in each integration, if it somehow errors in the callback and set of queues.

[Peripheral1994]

One thing I notice is that https://github.com/segmentio/analytics.js-core/blob/master/test/analytics.test.js seems to use the only variety of .ready(callback). I can't tell for sure what it intends and this might just be legacy behavior.

It don't see any production cases of calling .ready with any arguments, so we should be safe in that regard, though I agree that adding in a simple instanceof check should protect us in case this changes in the future.

[f2prateek]

Nice - I think the reasoning sound good to me.

Couple of notes:

• I don't think this would fix the AppCues integration specifically yet, since this one is on v2 (https://github.com/segment-integrations/analytics.js-integration-appcues/blob/master/package.json#L26). We'll want to backport this change to the 2.x branch like we did for Don't swallow integration errors #62. But the good thing is once Don't swallow integration errors #62 is live, we'll get a good baseline of what the current error rate is, and we should expect this change to reduce the error rate for appcues (in fact we should see a dip in almost all other integrations that load 3rd party scripts).

• I think this exacerbates the problem for Add the ability to specify ready for individual integrations analytics.js#409, since we're adding a new error case which would case the global ready callback to not be invoked. Ultimately we need to ship Add analytics.ready(name, fn) function analytics.js-core#33 to remedy this.

[calvinfo]

Thanks for the reviews guys! I hadn't realized either the 2.x branch or the .ready problem... I'll circle back here. One idea for the short-term is to update the Appcues code to now fire when an error is encountered. I'm not sure if I'll have enough cycles on-hand to go through and audit everything right now, but let me think on it for another day.

[nettofarah]

thanks for the update, @calvinfo.

[f2prateek]

I've added this to our JIRA (https://segment.atlassian.net/browse/LIB-372) so we can pick this up soon.

[dobesv]

We're running into this issue now. An Ad-Blocker blocks twitter ads from loading, and then none of our integrations load. Is there anything we can do to move this fix forward? Once this part of it is merged we would want to update the twitter integration itself to propagate errors back to analytics.js