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

Use page lifecycle APIs instead of window unload event (discussion needed) #43

Open
patrickshaughnessy opened this issue Oct 16, 2020 · 2 comments

Comments

@patrickshaughnessy
Copy link

I observed an issue using beaver logger where on navigation (redirect) to a new page, the beaver logger payload is fired, but reported as "Stalled" in Chrome network tab. The request did not complete and seems to not have made it to the server.

Digging a little deeper I came across the following:

  1. Beaver-logger optionally uses the sendBeacon API, but currently may be using the wrong events to detect when to send the beacon payload (unload / beforeunload).
  2. MDN docs call out some discussion about sendBeacon being potentially broken Comments section has an interesting discussion about whether it's a problem with sendBeacon or a with the actual vendor implementations not actually firing sendBeacon when they're supposed to.
  3. This article suggests that we should not rely on unload / beforeunload events, especially in mobile browsers, and this can cause a ton of missing logs.
  4. Google's recommendation is to never use the unload event in modern browsers!
  5. Google also provides a library for normalizing the page lifecycle API cross-browser

I'm still trying to digest all of these resources (and a little concerned about the dates on these articles), but wondering if it's worth considering using the page visibility API or the cross-browser Google library implementation of it anyways to detect when to automatically send the logs rather than the unload and beforeunload events.

@mnicpt
Copy link
Contributor

mnicpt commented Oct 19, 2020

@patrickshaughnessy

  1. So a couple of things here. The unload and beforeunload events basically flush any logs that haven't been sent in the last 60 seconds. Basically, if the browser doesn't support one of these well, the other is a backup. If beforeunload handles flushing the logs, then there won't be any if unload is handled. Given that, I'm wondering if enableSendBeacon is true, if we shouldn't send immediately rather than wait for the flush interval or require flush() to be called to immediately flush the log.
  2. We are aware of the following limitations of using sendBeacon, https://caniuse.com/?search=sendBeacon, and put the ownership on the user to enable this feature since it is off by default.
  3. Great article! We should consider this moving forward and update this repo to use these lifecycle states as well.
  4. I think using the above states as well as beforeunload and unload should ensure we catch it in one of them.
  5. Interesting! I'll consult with Daniel when he gets back from vacation. I'm not sure about bringing in this library as I think we can observe all possible states ourselves. But haven't looked at it real closely.

Thanks for bringing this up! Our goal in providing the ability to use sendBeacon was to eliminate loosing any logs and so this really helps us uphold this. Were you thinking about working on this and submitting a PR?

@patrickshaughnessy
Copy link
Author

@mnicpt Thanks for the reply! I'm seeing pageVisibility API supported almost across the board: https://caniuse.com/pagevisibility so maybe the Google library is not necessary anymore.

Since I'm not super familiar with this repo or these browser APIs and quirks, I'd probably want to sync up to understand the scope and expected outcomes before picking up this work. But yes, happy to take this on and implement after that.

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

No branches or pull requests

2 participants