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

Fix batching after page navigation #915

Merged
merged 6 commits into from
Aug 1, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/orange-actors-hug.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@segment/analytics-next': patch
---

Fix batching after page navigation
3 changes: 1 addition & 2 deletions .github/PULL_REQUEST_TEMPLATE
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,4 @@ Hello! And thanks for contributing to the Analytics-Next 🎉
Also make sure to describe how you tested this change, include any gifs or screenshots you find necessary.
--->


[] I've included a changeset (psst. run `yarn changeset`. Read about changesets [here](https://github.com/changesets/changesets/blob/main/docs/adding-a-changeset.md)).
- [ ] I've included a changeset (psst. run `yarn changeset`. Read about changesets [here](https://github.com/changesets/changesets/blob/main/docs/adding-a-changeset.md)).
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { onPageLeave } from '../on-page-leave'
import { onPageChange } from '../on-page-change'

const helpers = {
dispatchEvent(event: 'pagehide' | 'visibilitychange') {
Expand All @@ -24,42 +24,51 @@ beforeEach(() => {
helpers.setVisibilityState('visible')
})

describe('onPageLeave', () => {
describe('onPageChange', () => {
test('callback should fire on pagehide', () => {
const cb = jest.fn()
onPageLeave(cb)
onPageChange(cb)
helpers.dispatchPageHideEvent()
expect(cb).toBeCalledTimes(1)
})

test('callback should fire if document becomes hidden', () => {
const cb = jest.fn()
onPageLeave(cb)
onPageChange(cb)
helpers.dispatchVisChangeEvent('hidden')
expect(cb).toBeCalledTimes(1)
})

test('callback should *not* fire if document becomes visible', () => {
test('callback should fire if document becomes visible', () => {
const cb = jest.fn()
onPageLeave(cb)
onPageChange(cb)
helpers.dispatchVisChangeEvent('visible')
expect(cb).not.toBeCalled()
expect(cb).toBeCalledTimes(1)
})

test('if both event handlers fire, callback should still fire only once', () => {
const cb = jest.fn()
onPageLeave(cb)
onPageChange(cb)
helpers.dispatchVisChangeEvent('hidden')
helpers.dispatchPageHideEvent()
expect(cb).toBeCalledTimes(1)
})

test('if user leaves a tab, returns, and leaves again, callback should be called on each departure', () => {
test('if user leaves a tab, returns, and leaves again, callback should be called on each navigation', () => {
const cb = jest.fn()
onPageLeave(cb)
onPageChange(cb)
helpers.dispatchVisChangeEvent('hidden')
helpers.dispatchVisChangeEvent('visible')
helpers.dispatchVisChangeEvent('hidden')
expect(cb).toBeCalledTimes(2)
expect(cb).toBeCalledTimes(3)
})

test('if user navigates, callback should be passed appropriate "unloaded" value', () => {
const cb = jest.fn()
onPageChange(cb)
helpers.dispatchVisChangeEvent('hidden')
expect(cb).toBeCalledWith(true)
helpers.dispatchVisChangeEvent('visible')
expect(cb).toBeCalledWith(false)
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -7,23 +7,23 @@
*
* adapted from https://stackoverflow.com/questions/3239834/window-onbeforeunload-not-working-on-the-ipad/52864508#52864508,
*/
export const onPageLeave = (cb: (...args: unknown[]) => void) => {
export const onPageChange = (cb: (...args: boolean[]) => void) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:
This sig says that there are multiple bool args.
Type signature should just be (unloaded: boolean) => void

let unloaded = false // prevents double firing if both are supported

window.addEventListener('pagehide', () => {
if (unloaded) return
unloaded = true
cb()
cb(unloaded)
})

// using document instead of window because of bug affecting browsers before safari 14 (detail in footnotes https://caniuse.com/?search=visibilitychange)
document.addEventListener('visibilitychange', () => {
if (document.visibilityState == 'hidden') {
if (unloaded) return
unloaded = true
cb()
} else {
unloaded = false
}
cb(unloaded)
})
}
8 changes: 4 additions & 4 deletions packages/browser/src/plugins/segmentio/batched-dispatcher.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { SegmentEvent } from '../../core/events'
import { fetch } from '../../lib/fetch'
import { onPageLeave } from '../../lib/on-page-leave'
import { onPageChange } from '../../lib/on-page-change'

export type BatchingDispatchConfig = {
size?: number
Expand Down Expand Up @@ -91,10 +91,10 @@ export default function batch(
}, timeout)
}

onPageLeave(() => {
pageUnloaded = true
onPageChange((unloaded: boolean) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: No need to put "boolean" here; that should be part of the fn signature type

pageUnloaded = unloaded

if (buffer.length) {
if (pageUnloaded && buffer.length) {
const reqs = chunks(buffer).map(sendBatch)
Promise.all(reqs).catch(console.error)
}
Expand Down