{"action":"created","issue":{"url":"https://api.github.com/repos/segmentio/analytics.js-integration/issues/63","repository_url":"https://api.github.com/repos/segmentio/analytics.js-integration","labels_url":"https://api.github.com/repos/segmentio/analytics.js-integration/issues/63/labels{/name}","comments_url":"https://api.github.com/repos/segmentio/analytics.js-integration/issues/63/comments","events_url":"https://api.github.com/repos/segmentio/analytics.js-integration/issues/63/events","html_url":"https://github.com/segmentio/analytics.js-integration/pull/63","id":324587917,"number":63,"title":".ready: fix behavior when packages fail to load","user":{"login":"calvinfo","id":487539,"avatar_url":"https://avatars0.githubusercontent.com/u/487539?v=4","gravatar_id":"","url":"https://api.github.com/users/calvinfo","html_url":"https://github.com/calvinfo","followers_url":"https://api.github.com/users/calvinfo/followers","following_url":"https://api.github.com/users/calvinfo/following{/other_user}","gists_url":"https://api.github.com/users/calvinfo/gists{/gist_id}","starred_url":"https://api.github.com/users/calvinfo/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/calvinfo/subscriptions","organizations_url":"https://api.github.com/users/calvinfo/orgs","repos_url":"https://api.github.com/users/calvinfo/repos","events_url":"https://api.github.com/users/calvinfo/events{/privacy}","received_events_url":"https://api.github.com/users/calvinfo/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":4,"created_at":"2018-05-19T00:52:11Z","updated_at":"2018-05-21T16:59:14Z","closed_at":null,"author_association":"MEMBER","pull_request":{"url":"https://api.github.com/repos/segmentio/analytics.js-integration/pulls/63","html_url":"https://github.com/segmentio/analytics.js-integration/pull/63","diff_url":"https://github.com/segmentio/analytics.js-integration/pull/63.diff","patch_url":"https://github.com/segmentio/analytics.js-integration/pull/63.patch"},"body":"Netto brought an error with the Appcues integration up to my\r\nattention. Apparently when the script fails to load, it causes\r\nan exception which bubbles up to the top.\r\n\r\nI looked through the code and was pretty surprised by this, since\r\nthe Appcues integration itself is very few lines of code, and\r\nnothing out of the ordinary.\r\n\r\nWhen I dug in, I realized that this comes from an error where the\r\nAppcues javascript fails to load for some reason.\r\n\r\nWhen that happens here's the sequence of events:\r\n\r\n- segmentio/load-script gets called, and then calls a callback\r\n  with the error\r\n- this in turn calls .ready (in Appcues, but I think also many\r\n  other integrations)\r\n- this emits a ready handler, which then immediately starts\r\n  flushing the message queue, causing an uncaught exception\r\n\r\nThis fix instead causes .ready when called with an error to\r\n_not_ mark the integration as ready. Avoiding further errors.\r\n\r\nThe one thing I'm unsure of here is whether we ever end up calling\r\n.ready() with an argument–my hunch is 'no' given that it wouldn't\r\ndo anything, but it might be worth adding an instanceof Error check.\r\n\r\nThoughts?\r\n\r\nRelevant Appcues code: https://github.com/segment-integrations/analytics.js-integration-appcues/blob/master/lib/index.js#L27-L29\r\n\r\nRelevant a.js integration load code: https://github.com/segmentio/analytics.js-integration/blob/master/lib/protos.js#L237-L280"},"comment":{"url":"https://api.github.com/repos/segmentio/analytics.js-integration/issues/comments/390716136","html_url":"https://github.com/segmentio/analytics.js-integration/pull/63#issuecomment-390716136","issue_url":"https://api.github.com/repos/segmentio/analytics.js-integration/issues/63","id":390716136,"user":{"login":"calvinfo","id":487539,"avatar_url":"https://avatars0.githubusercontent.com/u/487539?v=4","gravatar_id":"","url":"https://api.github.com/users/calvinfo","html_url":"https://github.com/calvinfo","followers_url":"https://api.github.com/users/calvinfo/followers","following_url":"https://api.github.com/users/calvinfo/following{/other_user}","gists_url":"https://api.github.com/users/calvinfo/gists{/gist_id}","starred_url":"https://api.github.com/users/calvinfo/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/calvinfo/subscriptions","organizations_url":"https://api.github.com/users/calvinfo/orgs","repos_url":"https://api.github.com/users/calvinfo/repos","events_url":"https://api.github.com/users/calvinfo/events{/privacy}","received_events_url":"https://api.github.com/users/calvinfo/received_events","type":"User","site_admin":false},"created_at":"2018-05-21T16:59:14Z","updated_at":"2018-05-21T16:59:14Z","author_association":"MEMBER","body":"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."}}
{"actor":{"display_login":"calvinfo"}}