feat: allow configuring request queue interval #1708
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR adds configurable request queue intervals to the PostHog JS library, allowing more frequent event batching for scenarios with short page visits or automated redirects.
- Added
request_queue_config.flush_interval_msin/src/types.tsto configure batch intervals between 250-5000ms (default 3000ms) - Implemented interval clamping with validation in
/src/request-queue.tsusingclampToRangeutility - Renamed request types for clarity (e.g.
RequestOptionstoRequestWithOptions) across multiple files for consistency - Added comprehensive test coverage in
/src/__tests__/request-queue.test.tsfor interval bounds and configuration integration - Maintained backward compatibility while adding new configuration options for request batching
7 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
| it('handles poll after enqueueing requests', () => { | ||
| queue.enqueue({ | ||
| data: { event: 'foo', timestamp: EPOCH - 3000 }, | ||
| transport: 'XHR', | ||
| url: '/e', | ||
| }) | ||
| queue.enqueue({ | ||
| data: { event: '$identify', timestamp: EPOCH - 2000 }, | ||
| url: '/identify', | ||
| }) | ||
| queue.enqueue({ | ||
| data: { event: 'bar', timestamp: EPOCH - 1000 }, | ||
| url: '/e', | ||
| }) | ||
| queue.enqueue({ | ||
| data: { event: 'zeta', timestamp: EPOCH }, | ||
| url: '/e', | ||
| batchKey: 'sessionRecording', | ||
| }) | ||
|
|
||
| queue.unload() | ||
| queue.enable() | ||
|
|
||
| expect(sendRequest).toHaveBeenCalledTimes(3) | ||
| expect(sendRequest).toHaveBeenCalledTimes(0) | ||
|
|
||
| expect(sendRequest).toHaveBeenNthCalledWith(1, { | ||
| data: [ | ||
| { event: 'foo', timestamp: 1610000000 }, | ||
| { event: 'bar', timestamp: 1630000000 }, | ||
| ], | ||
| transport: 'sendBeacon', | ||
| url: '/e', | ||
| jest.runOnlyPendingTimers() | ||
|
|
||
| expect(sendRequest).toHaveBeenCalledTimes(3) | ||
| expect(jest.mocked(sendRequest).mock.calls).toEqual([ | ||
| [ | ||
| { | ||
| url: '/e', | ||
| data: [ | ||
| { event: 'foo', offset: 3000 }, | ||
| { event: 'bar', offset: 1000 }, | ||
| ], | ||
| transport: 'XHR', | ||
| }, | ||
| ], | ||
| [ | ||
| { | ||
| url: '/identify', | ||
| data: [{ event: '$identify', offset: 2000 }], | ||
| }, | ||
| ], | ||
| [ | ||
| { | ||
| url: '/e', | ||
| data: [{ event: 'zeta', offset: 0 }], | ||
| batchKey: 'sessionRecording', | ||
| }, | ||
| ], | ||
| ]) | ||
| }) |
There was a problem hiding this comment.
style: This test now lives under 'with default config' but actually tests core queue functionality. Consider moving to a separate describe block for clarity.
| { event: 'foo', timestamp: 1610000000 }, | ||
| { event: 'bar', timestamp: 1630000000 }, |
There was a problem hiding this comment.
style: Inconsistent timestamp formatting - some use underscores (1_610_000_000) while these don't. Should standardize for readability.
|
Size Change: +1.53 kB (+0.05%) Total Size: 3.3 MB
ℹ️ View Unchanged
|
| import { each } from './utils' | ||
|
|
||
| import { isArray, isUndefined } from './utils/type-utils' | ||
| import { clampToRange } from './utils/number-utils' | ||
|
|
||
| export const DEFAULT_FLUSH_INTERVAL_MS = 3000 |
There was a problem hiding this comment.
Feels like a solution that doesn't actually relate to the problem...
The problem sounds more to me like customers want a way to flush data and wait for the response before unloading?
There was a problem hiding this comment.
In the specific case they have an auto-refresh after 2 seconds, so they wouldn't want to delay too long
it's also a much bigger change to wait on refreshes...
what I'd love to do instead is have this queue store to local storage as well as memory, and then this all becomes much safer
(so, yep, this is for sure a workaround I can give this customer quickly, much more than something I'd expect many people to set)
the request queue has a fixed interval where it sends events, this allows us to batch events to reduce load on ingestion and reduce network calls on client devices
but for customers where they may have short page visits or automated page redirects this means relying on the unload mechanism which browsers don't guarantee
let's allow configuring that interval between 250 and 5000ms