search

segment-integrations / analytics.js-integration-appboy

The Appboy analytics.js integration
https://segment.com/docs/integrations/appboy/
MIT License
0 stars 6 forks source link

integration snippet needs update (or replacement with npm require) #33

Open froodian opened 6 years ago

froodian commented 6 years ago

The current integration is using a loading snippet at https://github.com/segment-integrations/analytics.js-integration-appboy/blob/fd6b5cdd65398c6cdaddb30d4f952e5a5de35384/lib/index.js#L124 that's somewhat different from the default one at https://github.com/Appboy/appboy-web-sdk#getting-started, and it's causing loading failures (js errors) in some environments. The issue is that it explicitly gives appboy and appboyQueue the window namespace (window.appboy, window.appboyQueue), but pushes to the appboyQueue in the global namespace. There were some updates on the Braze end around this snippet a while back to improve this situation, so it's possible it's simply outdated.

A larger point I want to raise here though is that this entire snippet with the appboyQueue structure was designed to be put in the <head> of an HTML page for naive clients in a raw html environment, and exists to handle method calls that happen before the script has loaded. Given that in this integration, the script is instead loaded with this.load('v2', function() {, that snippet could be removed entirely and all methods on appboy simply called within the callback function.

Even simpler in fact, given the npm environment, would be to do npm install --save-dev appboy-web-sdk, var appboy = require('appboy-web-sdk'); and then you can just immediately call appboy.initialize and start calling methods from there. I'm not sure how you'd want to handle your v1 and v2 support however.

Given that there are a few different options here, I leave it to you to choose which one you want in your codebase, but that mismatch between window.appboyQueue= and appboyQueue.push should be rectified somehow, and I can tell you that internally the SDK draws from appboyQueue if it exists (and ignores it gracefully if it doesn't), so this code should either define and push to that, or omit it entirely.

Thanks!

ucarion commented 6 years ago

@froodian Thanks for the really detailed explanation! Really helpful. :)

If I understand you correctly, it sounds like, at the very least for the v2 SDK, that we could remove the Appboy snippet altogether?

f2prateek commented 6 years ago

You mentioned npm install --save-dev appboy-web-sdk. Curious why the dev option? Shouldn't this be a regular runtime dependency?

dwillislc commented 6 years ago

@ucarion, yup you can just remove the Appboy snippet altogether for v2 of the SDK and follow the code found here: https://github.com/Appboy/appboy-web-sdk#alternative-npm-installation

@f2prateek i believe you're right in that you should just run npm install --save appboy-web-sdk without include the dev option. (@froodian sanity)

ghost commented 6 years ago

Yes, npm install --save appboy-web-sdk is appropriate here.

froodian commented 6 years ago

yeah, sorry about that, --save-dev is just an old habit of mine - --save is more appropriate, and yup, I think you could just use npm for v2 and leave v1 untouched if you wanted

(and thanks to the other braze folks for filling in / answering for me while I was out yesterday)

SegmentDestinationsBot commented 5 years ago

Hi @froodian, as part of the monorepo migration, this issue has been moved to new issue. Our engineers have been notified and will prioritize and work on it ASAP. Thank you!

For more information, see README.md.