feat: add sendBeacon support by ajhorst · Pull Request #412 · amplitude/Amplitude-JavaScript

ajhorst · 2021-07-27T16:24:12Z

Summary

add support for using sendBeacon instead of xhr to send data
add a convenience method option to log with beacon when a user exits the browser

Checklist

Does your PR title have the correct title format?
Does your PR have a breaking change?: No

- add transport as option (either http or beacon) - support sending event with either transport mechanism

ajhorst · 2021-07-29T21:50:47Z

Updated the confluence doc on sendBeacon to describe what I'm going for with this PR: https://amplitude.atlassian.net/wiki/spaces/sdk/pages/1492058287/Beacon+API+-+JS+SDK+Proposal#Implementation%3A

lgtm-com · 2021-07-30T22:20:34Z

This pull request introduces 28 alerts when merging 7902a98 into 564f656 - view on LGTM.com

new alerts:

9 for Property access on null or undefined
4 for Useless conditional
4 for Incomplete string escaping or encoding
3 for Unneeded defensive code
2 for Useless assignment to local variable
2 for Duplicate character in character class
2 for Unused variable, import, function or class
2 for Insecure randomness

kelvin-lu · 2021-08-10T10:26:32Z

    checksum: md5(Constants.API_VERSION + this.options.apiKey + events + uploadTime),
  };

+  if (!navigator.sendBeacon) {


hm, I think I would expect this in the validateTransport function and not in sendEvents

kelvin-lu · 2021-08-10T10:27:39Z

+        const handleVisibilityChange = () => {
+          const prevTransport = this.options.transport;
+          this.setTransport(Constants.TRANSPORT_BEACON);
+          onExitPage();


thoughts on hooking the current object with onExitPage(this) allowing users to access the client earlier?

As in, so they can write the callback on a line before initializing the client with it? The way I was testing it, as long as the amplitude object is in scope within the file, it doesn't need to be initialized until onExitPage is called. So the following is fine:

const onExitPage = () => { amplitude.getInstance().logEvent('logging onExitPage'); }; amplitude.getInstance().init('API_KEY', userID, { onExitPage });

But if there's another benefit you're thinking of by making the client an argument, it would be an easy change to make

It might make it easier for people using the snippet to use the full client obj (incl. "private" methods and the full list of options post-init to construct event properties) but you are right that everything publicly supported is available.

I don't feel too strongly about this though so we can not do this and see what people actually want as arguments to this option!

kelvin-lu · 2021-08-10T10:28:38Z

+    this.options.transport = Constants.TRANSPORT_HTTP;
+  }
+
+  if (this.options.transport === Constants.TRANSPORT_BEACON) {


might be too much, but would be good to replace a lot of these with a small descriptive helper function like doesTransportSupportSaveEvents

I tried adding helper functions, but since there are only two transports now, they didn't add much descriptivity compared to checking the value itself. If there end up being more than two, then definitely there should be helper functions to categorize them like this

kelvin-lu · 2021-08-10T10:29:15Z

  return true;
 };

+const validateTransport = function validateTransport(transport) {


is this checked on initialization? In case it comes in bad

it's not but it should be, good call

kelvin-lu

Not super familiar with our build system as of now, but why is this PR adding 2 5k sized files?

ajhorst · 2021-08-11T17:31:14Z

@kelvin-lu those files were built while I was testing this PR locally and weren't in the gitignore, so I accidentally committed them. I just now took them out of the PR and added them to the gitignore

kelvin-lu

overall lgtm!

I think the only other thing DX wise is some sort of warning that the Transport Beacon does not retry like regular XHR requests. I feel like that might be a source of questions/data governance concerns

kelvin-lu · 2021-08-13T17:40:44Z

+    return this._q.push(['setTransport'].concat(Array.prototype.slice.call(arguments, 0)));
+  }
+
+  const t = transport.toLowerCase();


not sure how to manage this in validateTransport but you should probably check that this is a string before using toLowerCase

perhaps like validateProperties we coudl have validateTransport return a sanitary version of the string? :)

ajhorst · 2021-08-13T17:49:12Z

That's an interesting point RE DX. I have a followup ticket to update the dev docs with instructions for sendBeacon, so I'll definitely mention the lack of retry.

ajhorst · 2021-08-13T17:50:29Z

I'll tighten up validateTransport like you suggest and then merge

# [8.5.0](v8.4.0...v8.5.0) (2021-08-13) ### Bug Fixes * LGTM return fix for falsy str ([#416](#416)) ([f744fe7](f744fe7)) ### Features * add sendBeacon support ([#412](#412)) ([0517038](0517038))

AJ Horst added 3 commits July 26, 2021 22:17

feat(beacon): add sendBeacon support

80744cf

- add transport as option (either http or beacon) - support sending event with either transport mechanism

feat(beacon): add support for onExitPage handler

f708c94

feat(beacon): add tests

698df2a

ajhorst requested review from jooohhn and kelvin-lu and removed request for jooohhn July 27, 2021 16:24

feat(beacon): add clarifying comment

9e7d1f6

ajhorst changed the title ~~Amp 13223 beacon support~~ feat(beacon): add sendBeacon support (AMP-13223) Jul 27, 2021

ajhorst changed the title ~~feat(beacon): add sendBeacon support (AMP-13223)~~ feat(beacon): add sendBeacon support Jul 27, 2021

ajhorst changed the title ~~feat(beacon): add sendBeacon support~~ feat: add sendBeacon support Jul 27, 2021

ajhorst added 2 commits July 30, 2021 14:58

Merge branch 'main' into AMP-13223_beacon_support

0222136

add error callback logic to sendBeacon'

7902a98

ajhorst changed the title ~~feat: add sendBeacon support~~ feat: add sendBeacon support Jul 30, 2021

ajhorst requested review from tevanko and removed request for tevanko August 9, 2021 18:27

kelvin-lu reviewed Aug 10, 2021

View reviewed changes

ajhorst added 3 commits August 11, 2021 10:26

move transport validation to init and check browser in validate function

c2a6222

remove unneeded build files

006352c

add new build files to gitignore

bfad086

ajhorst added 2 commits August 11, 2021 13:30

Merge branch 'main' into AMP-13223_beacon_support

8646f90

fix lint issue from merge with master

ecce8b6

ajhorst requested a review from kelvin-lu August 13, 2021 17:29

kelvin-lu approved these changes Aug 13, 2021

View reviewed changes

remove toLowerCase check that may error out if input is not string

e989cb2

ajhorst merged commit 0517038 into main Aug 13, 2021

ajhorst deleted the AMP-13223_beacon_support branch August 13, 2021 22:04

Conversation

ajhorst commented Jul 27, 2021

Summary

Checklist

Uh oh!

ajhorst commented Jul 29, 2021

Uh oh!

lgtm-com Bot commented Jul 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kelvin-lu left a comment

Choose a reason for hiding this comment

Uh oh!

ajhorst commented Aug 11, 2021

Uh oh!

kelvin-lu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajhorst commented Aug 13, 2021

Uh oh!

ajhorst commented Aug 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants