-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
add beacon transport #1498
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 beacon transport #1498
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1498 +/- ##
==========================================
+ Coverage 72.55% 72.61% +0.06%
==========================================
Files 41 41
Lines 1246 1249 +3
Branches 238 239 +1
==========================================
+ Hits 904 907 +3
Misses 333 333
Partials 9 9
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL Beacon API 😅 🙈
This is super nice, thanks for that.
Also, you are right, no need creating a new transport instance every time, I will change that.
thanks @HazAT , do we know if this will be included in the next |
@tvanier Preparing an |
Why did you cancel beacon from version 5.0? |
Because it was partially broken, see: #1952
But I see it has been fixed somewhere in 2020, so we may think again about reintroducing it (which is still not trivial, as old chrome versions would still break anyway). |
It has been a while since I worked with the Beacon API, I was wondering if, knowing the Chrome change was likely to be temporary, the code for beacon transport could have been kept, changing just export function supportsBeacon(): boolean {
const global = getGlobalObject();
if ('navigator' in global && 'sendBeacon' in global.navigator) {
try {
const blob = new Blob([JSON.stringify('{}')], {type : 'application/json'});
global.navigator.sendBeacon('/', blob); // or any better url
// no exception
return true;
} catch {
// exception in Chrome 59
}
}
return false;
} Or just |
We'll rethink the beacon transport once we get sessions, tracing and performance measurements sorted out for sure, as it'll be really useful there. For now, we don't want to add more bundle size for features that only few people might make use of. |
Provide a new transport based on the browser Beacon API, supported in all major browsers except IE (caniuse). Using
navigator.sendBeacon
instead offetch
orXMLHttpRequest
could help sending reports at unload time, and possibly allow for bigger reports to be sent.This pull request adds
BeaconTransport
to thebrowser
package, leaving it as an opt-in:If working good, beacon transport could be the default in the future? In packages/browser/src/backend.ts:
BTW does transport needs to be instantiated at every
sendEvent
?