Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add optional navigator.sendBeacon transport #240

Merged
merged 8 commits into from Jan 14, 2020
Merged

add optional navigator.sendBeacon transport #240

merged 8 commits into from Jan 14, 2020

Conversation

tdumitrescu
Copy link
Member

@tdumitrescu tdumitrescu commented Jan 10, 2020

fixes #184

Several ways to use it:

// for an individual track() call
mixpanel.track('my event', {my: 'props'}, {transport: 'sendBeacon'});

// turn on for every Mixpanel call when page is unloading
// (you would use this to use sendBeacon for everything, including
// mixpanel.people calls)
window.addEventListener(`unload`, function() {
  mixpanel.set_config({api_transport: 'sendBeacon'});
  mixpanel.track('my event');
  mixpanel.people.set({foo: 'bar'});
});

// initialize for all tracking; not recommended as it will prevent any
// request-retry facilities
mixpanel.init('my token', {api_transport: 'sendBeacon'});
mixpanel.track('my event');

evnp
evnp approved these changes Jan 11, 2020
var transport = options['transport'];
if (transport) {
options.transport = transport;
}
Copy link
Contributor

@evnp evnp Jan 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused by this, missing how it modifies options. Can you add a clarifying comment if it's intentional?

Copy link
Member Author

@tdumitrescu tdumitrescu Jan 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do - it's because track() is a public API and so "transport" shouldn't be minified, whereas _send_request() is internal and so its options.transport can be minified however closure compiler wants.

Copy link
Contributor

@evnp evnp Jan 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that makes a strange amount of sense, thanks for the explanation

var sendBeacon = navigator['sendBeacon'];
if (sendBeacon) {
sendBeacon = _.bind(sendBeacon, navigator);
}
Copy link
Contributor

@evnp evnp Jan 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd personally find the code below more readable if I saw navigator.sendBeacon, provides more context even though it's more verbose and you wouldn't need this bind iiuc. Nbd if you prefer it this way.

Copy link
Contributor

@evnp evnp Jan 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just realized you probably need this to avoid the minifier messing with 'sendBeacon'; disregard my comment

Copy link
Member Author

@tdumitrescu tdumitrescu Jan 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another size optimization, like we do throughout this SDK. It lets closure compiler compress this down by referring to a minified var rather than injecting "navigator.sendBeacon" every time it's used. The minified version of this code is:

U=I.sendBeacon;U&&(U=c.bind(U,I));

so the code below can just refer to U

@tdumitrescu tdumitrescu merged commit f97369d into master Jan 14, 2020
1 check passed
@tdumitrescu tdumitrescu deleted the sendbeacon branch Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants