Skip to content

ref(replay): Replace lodash.debounce with custom debounce implementation #6593

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

Merged
merged 7 commits into from
Jan 4, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
11 changes: 2 additions & 9 deletions packages/browser/src/client.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,6 @@
import { BaseClient, getEnvelopeEndpointWithUrlEncodedAuth, Scope, SDK_VERSION } from '@sentry/core';
import {
BrowserClientReplayOptions,
ClientOptions,
Event,
EventHint,
Options,
Severity,
SeverityLevel,
} from '@sentry/types';
import type { BrowserClientReplayOptions } from '@sentry/types';
import { ClientOptions, Event, EventHint, Options, Severity, SeverityLevel } from '@sentry/types';
import { createClientReportEnvelope, dsnToString, logger, serializeEnvelope } from '@sentry/utils';

import { eventFromException, eventFromMessage } from './eventbuilder';
Expand Down
2 changes: 0 additions & 2 deletions packages/replay/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,8 @@
"homepage": "https://docs.sentry.io/platforms/javascript/session-replay/",
"devDependencies": {
"@babel/core": "^7.17.5",
"@types/lodash.debounce": "4.0.7",
"@types/pako": "^2.0.0",
"jsdom-worker": "^0.2.1",
"lodash.debounce": "4.0.8",
"pako": "^2.0.4",
"rrweb": "1.1.3",
"tslib": "^1.9.3"
Expand Down
3 changes: 0 additions & 3 deletions packages/replay/rollup.bundle.config.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import commonjs from '@rollup/plugin-commonjs';
import replace from '@rollup/plugin-replace';

import { makeBaseBundleConfig, makeBundleConfigVariants } from '../../rollup/index.js';
Expand All @@ -19,8 +18,6 @@ const baseBundleConfig = makeBaseBundleConfig({
__SENTRY_REPLAY_VERSION__: JSON.stringify(pkg.version),
},
}),
// lodash.debounce is a CJS module, so we need to convert it to ESM first
commonjs(),
Comment on lines -22 to -23
Copy link
Member Author

Choose a reason for hiding this comment

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

Since build changes are always risky, I double checked NPM packages and CDN bundles and they still work with my test apps.

],
},
});
Expand Down
3 changes: 0 additions & 3 deletions packages/replay/rollup.npm.config.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import path from 'path';

import commonjs from '@rollup/plugin-commonjs';
import replace from '@rollup/plugin-replace';

import { makeBaseNPMConfig, makeNPMConfigVariants } from '../../rollup/index';
Expand All @@ -19,8 +18,6 @@ export default makeNPMConfigVariants(
__SENTRY_REPLAY_VERSION__: JSON.stringify(pkg.version),
},
}),
// lodash.debounce is a CJS module, so we need to convert it to ESM first
commonjs(),
],
output: {
// set exports to 'named' or 'auto' so that rollup doesn't warn about
Expand Down
7 changes: 3 additions & 4 deletions packages/replay/src/replay.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
import { addGlobalEventProcessor, captureException, getCurrentHub, setContext } from '@sentry/core';
import { Breadcrumb, Event } from '@sentry/types';
import { addInstrumentationHandler, logger } from '@sentry/utils';
import debounce from 'lodash.debounce';
import { EventType, record } from 'rrweb';

import {
Expand Down Expand Up @@ -45,6 +44,7 @@ import { createBreadcrumb } from './util/createBreadcrumb';
import { createPayload } from './util/createPayload';
import { createPerformanceSpans } from './util/createPerformanceSpans';
import { createReplayEnvelope } from './util/createReplayEnvelope';
import { debounce } from './util/debounce';
import { getReplayEvent } from './util/getReplayEvent';
import { isExpired } from './util/isExpired';
import { isSessionExpired } from './util/isSessionExpired';
Expand Down Expand Up @@ -859,7 +859,6 @@ export class ReplayContainer implements ReplayContainerInterface {
// A flush is about to happen, cancel any queued flushes
this._debouncedFlush?.cancel();

// No existing flush in progress, proceed with flushing.
// this._flushLock acts as a lock so that future calls to `flush()`
// will be blocked until this promise resolves
if (!this._flushLock) {
Expand Down Expand Up @@ -892,8 +891,8 @@ export class ReplayContainer implements ReplayContainerInterface {
*/
flushImmediate(): Promise<void> {
this._debouncedFlush();
// `.flush` is provided by lodash.debounce
return this._debouncedFlush.flush();
// `.flush` is provided by the debounced function, analogously to lodash.debounce
return this._debouncedFlush.flush() as Promise<void>;
}

/**
Expand Down
70 changes: 70 additions & 0 deletions packages/replay/src/util/debounce.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
type DebouncedCallback = {
flush: () => void | unknown;
cancel: () => void;
(): void | unknown;
};
type CallbackFunction = () => unknown;
type DebounceOptions = { maxWait?: number };

/**
* Heavily simplified debounce function based on lodash.debounce.
*
* This function takes a callback function (@param fun) and delays its invocation
* by @param wait milliseconds. Optionally, a maxWait can be specified in @param options,
* which ensures that the callback is invoked at least once after the specified max. wait time.
*
* @param func the function whose invocation is to be debounced
* @param wait the minimum time until the function is invoked after it was called once
* @param options the options object, which can contain the `maxWait` property
*
* @returns the debounced version of the function, which needs to be called at least once to start the
* debouncing process. Subsequent calls will reset the debouncing timer and, in case @paramfunc
* was already invoked in the meantime, return @param func's return value.
* The debounced function has two additional properties:
* - `flush`: Invokes the debounced function immediately and returns its return value
* - `cancel`: Cancels the debouncing process and resets the debouncing timer
*/
export function debounce(func: CallbackFunction, wait: number, options?: DebounceOptions): DebouncedCallback {
let callbackReturnValue: unknown;

let timerId: ReturnType<typeof setTimeout> | undefined;
let maxTimerId: ReturnType<typeof setTimeout> | undefined;

const maxWait = options && options.maxWait ? Math.max(options.maxWait, wait) : 0;

function invokeFunc(): unknown {
cancelTimers();
callbackReturnValue = func();
return callbackReturnValue;
}

function cancelTimers(): void {
timerId !== undefined && clearTimeout(timerId);
maxTimerId !== undefined && clearTimeout(maxTimerId);
timerId = maxTimerId = undefined;
}

function flush(): unknown {
if (timerId !== undefined || maxTimerId !== undefined) {
return invokeFunc();
}
return callbackReturnValue;
}

function debounced(): unknown {
if (timerId) {
clearTimeout(timerId);
}
timerId = setTimeout(invokeFunc, wait);

if (maxWait && maxTimerId === undefined) {
maxTimerId = setTimeout(invokeFunc, maxWait);
}

return callbackReturnValue;
}

debounced.cancel = cancelTimers;
debounced.flush = flush;
return debounced;
}
14 changes: 8 additions & 6 deletions packages/replay/test/unit/index-errorSampleRate.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ describe('Replay (errorSampleRate)', () => {
expect(replay).not.toHaveLastSentReplay();

captureException(new Error('testing'));
jest.runAllTimers();
jest.advanceTimersByTime(5000);
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to replace a couple of runAllTimers calls with advanceTimersByTime because apparently, two timers don't seem to work with runAllTimers. I'm honestly not entirely sure why and I don't fully understand what might be causing these errors but I think correctness-wise, the debounce function isn't responsible for the test fails.

WDYT, does this change make sense? @billyvg you know these tests best. Is there something I'm missing?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure, I would think runAllTimers should work -- in any case I think replacing it with advanceTimersByTime is fine.

Should we replace 5000 (and the default for flushMinDelay) with a constant?

Copy link
Member Author

Choose a reason for hiding this comment

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

My theory is that the cancellation of the two timers when one of them is invoked might somehow interfere with whatever runAllTimers is doing but tbh this is a guess at best.

Should we replace 5000 (and the default for flushMinDelay) with a constant?

Sure can do

Copy link
Member Author

@Lms24 Lms24 Dec 22, 2022

Choose a reason for hiding this comment

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

Sure can do

On second thought, let's do this in a separate PR, just to not clutter this one. I've checked and we're using 5000 quite a lot in tests.

Edit: #6612

await new Promise(process.nextTick);

expect(replay).toHaveSentReplay({
Expand Down Expand Up @@ -99,8 +99,7 @@ describe('Replay (errorSampleRate)', () => {
events: JSON.stringify([{ data: { isCheckout: true }, timestamp: BASE_TIMESTAMP + 5020, type: 2 }]),
});

jest.runAllTimers();
await new Promise(process.nextTick);
jest.advanceTimersByTime(5000);

// New checkout when we call `startRecording` again after uploading segment
// after an error occurs
Expand All @@ -118,8 +117,10 @@ describe('Replay (errorSampleRate)', () => {
domHandler({
name: 'click',
});
jest.runAllTimers();

jest.advanceTimersByTime(5000);
await new Promise(process.nextTick);

expect(replay).toHaveLastSentReplay({
events: JSON.stringify([
{
Expand Down Expand Up @@ -297,7 +298,7 @@ describe('Replay (errorSampleRate)', () => {

captureException(new Error('testing'));

jest.runAllTimers();
jest.advanceTimersByTime(5000);
await new Promise(process.nextTick);

expect(replay).toHaveSentReplay({
Expand Down Expand Up @@ -398,7 +399,8 @@ it('sends a replay after loading the session multiple times', async () => {
expect(replay).not.toHaveLastSentReplay();

captureException(new Error('testing'));
jest.runAllTimers();

jest.advanceTimersByTime(5000);
await new Promise(process.nextTick);

expect(replay).toHaveSentReplay({
Expand Down
Loading