openstax / event-capture-client

GNU Affero General Public License v3.0
0 stars 0 forks source link

Flush events before closing the window #6

Open PiotrKozlowski opened 3 years ago

PiotrKozlowski commented 3 years ago

In https://github.com/openstax/event-capture-client/blob/cb363db59872585c6b48da1094f6a6d452f075f6/src/lib/capture.ts#L66 there is a fragment that I think is supposed to run the flush if user for example switches the tabs. Am I right?

if (document) {
    document.addEventListener('visibilitychange', () => {
      if (document.visibilityState === 'hidden') {
        flush.run();
      }
    });
  }

If I understand correctly we are flushing events only once per batchInterval which is 1 minute by default. This means that we will most likely lose some of the last events data.

Should we have a similar event listener for closing the tab? I was thinking about onbeforeunload but I'm not sure if the API call will not be stopped when we call it in there.

Maybe we could write something tricky that will detect user mouse movement and tell if a user wants to close the browser? This will not work for users that closes the window with a keyboard.

TomWoodward commented 3 years ago

flush.run(); bypasses the interval and runs synchronously as long as a client is defined.

when i wrote this i was doing research on onbeforeunload and a visibility listener was recommended instead

source https://developer.mozilla.org/en-US/docs/Web/API/Navigator/sendBeacon

The navigator.sendBeacon() method asynchronously sends a small amount of data over HTTP to a web server. It’s intended to be used in combination with the visibilitychange event (but not with the unload and beforeunload events).
PiotrKozlowski commented 3 years ago

That wasn't clear to me when I read the docs about visibilitychange event I see the info about safari though: https://developer.mozilla.org/en-US/docs/Web/API/Document/visibilitychange_event

Safari doesn’t fire visibilitychange as expected when the value of the visibilityState property transitions to hidden; so for that case, you need to also include code to listen for the pagehide event.

Do we want to handle this case?

TomWoodward commented 3 years ago

We should probably add something to handle that yea

On Tue, Feb 2, 2021, 10:43 AM Piotr Kozłowski notifications@github.com wrote:

That wasn't clear to me when I read the docs about visibilitychange event I see the info about safari though: https://developer.mozilla.org/en-US/docs/Web/API/Document/visibilitychange_event

Safari doesn’t fire visibilitychange as expected when the value of the visibilityState property transitions to hidden; so for that case, you need to also include code to listen for the pagehide event.

Do we want to handle this case?

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/openstax/event-capture-client/issues/6#issuecomment-771726426, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAE3KQ2VOCBNUK6NC7CV2GLS5AMRBANCNFSM4W6Z5RFQ .