ref(replay): Replace `lodash.debounce` with custom debounce implementation by Lms24 · Pull Request #6593 · getsentry/sentry-javascript

Lms24 · 2022-12-21T12:31:50Z

We've been using lodash.debounce to debounce the flushing of replay events. This has a number of drawbacks:

It caused bugs and build errors for our users ([Replay] Uncaught TypeError: debounce.default is not a function #6474, fix(replay): Ensure lodash.debounce does not trigger next.js warning #6551)
It's a third-party dependency and we generally want to use as few of them as possible
lodash's implementation includes features we don't use, making it's bundle size footprint bigger than it needs to be
lodash's implementation is in CJS, so we need to upcomiple it to ESM when building the replay package and CDN bundles

This PR replaces lodash's implementation with a heavily simplified version of debounce which just does what we need it for:

Delay the function invocation by a wait time and gate it with a maxWait value
Provide the return value of the invocation for subsequent debounced function calls
Provide a flush and cancel function on the debounced function (analogously to lodash).
Invoke the function after the wait/maxwait time triggered (i.e. on the trailing edge). Lodash allows to choose between the leading and trailing edge, which makes the implementation much more complicated than for us necessary
This works for functions without parameters. By not supporting args, we can further cut down bundle size. (We might want to revisit this in the future but for now, this should do).

With this change, we can also get rid of the package patch we introduced in #6551 and of the commonjs() plugin in our build process.

closes #6553

packages/utils/src/debounce.ts

github-actions · 2022-12-21T12:50:09Z

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	19.79 KB (+0.01% 🔺)
@sentry/browser - ES5 CDN Bundle (minified)	61.3 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	18.57 KB (-0.01% 🔽)
@sentry/browser - ES6 CDN Bundle (minified)	54.84 KB (0%)
@sentry/browser - Webpack (gzipped + minified)	20.34 KB (0%)
@sentry/browser - Webpack (minified)	66.48 KB (0%)
@sentry/react - Webpack (gzipped + minified)	20.36 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified)	47.56 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	26.75 KB (-0.02% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	25.19 KB (+0.02% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified)	41.59 KB (-1.18% 🔽)
@sentry/replay - Webpack (gzipped + minified)	37.6 KB (-1.49% 🔽)

lforst · 2022-12-21T13:11:11Z

Just a thought: Intuitively I would have implemented the debounce + maxWait by starting two timeouts. The first one is for maxWait. That timeout invokes the function if it hasn't been invoked yet and cancels the other timeout. The other timeout is for the actual debounce functionality.

With that implementation, I think we wouldn't have to track the remaining time and things like that. Was there a particular reason why you went this route? I might be missing something.

Lms24 · 2022-12-21T13:23:37Z

This was also my first approach and I already implemented it that way. However, I got replay test errors I couldn't explain, which made me look at the lodash implementation more carefully and I decided to give the one-timeout approach a try. Looking back though, I think I might have missed some details in my first implementation so I'll give the 2-timeouts approach another go. In case I don't get anywhere with it, I'll come back to this version.

Lms24

Ok, I've rewritten the debounce implementation with two timers, which should bring down bundle size more than the first version. This required some test adjustments (see my comment)

Also, I moved the function back to the Replay package because we can still pull it into utils if other packages need it.

Lms24 · 2022-12-22T13:04:24Z

packages/replay/test/unit/index-errorSampleRate.test.ts


    captureException(new Error('testing'));
-    jest.runAllTimers();
+    jest.advanceTimersByTime(5000);


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?

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?

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

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

Lms24 · 2022-12-22T13:07:33Z

packages/replay/rollup.bundle.config.js

-      // lodash.debounce is a CJS module, so we need to convert it to ESM first
-      commonjs(),


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

mydea

Great work! 🚀

Lms24 requested review from billyvg, lforst and mydea December 21, 2022 12:32

Lms24 force-pushed the lms-replay-debounce branch from d543172 to ee523b8 Compare December 21, 2022 12:36

lforst reviewed Dec 21, 2022

View reviewed changes

packages/utils/src/debounce.ts Outdated Show resolved Hide resolved

Lms24 added 4 commits December 22, 2022 10:16

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

10e0288

remove lodash.debounce dependencies and patch

df01c9f

remove leftover console logs

584f294

use 2-timers approach and adjust tests

b7ae986

Lms24 force-pushed the lms-replay-debounce branch from 6739731 to b7ae986 Compare December 22, 2022 12:52

Lms24 commented Dec 22, 2022

View reviewed changes

remove commonjs() rollup plugin

8b0bc87

Lms24 force-pushed the lms-replay-debounce branch from 964a080 to 8b0bc87 Compare December 22, 2022 13:08

Lms24 added 2 commits December 22, 2022 14:15

remove accidental line copy

853b98a

add test for equal wait and maxWait

816309b

Lms24 mentioned this pull request Dec 22, 2022

ref(replay): Extract flush min and max delay default values to constants #6612

Merged

mydea approved these changes Jan 3, 2023

View reviewed changes

billyvg approved these changes Jan 3, 2023

View reviewed changes

Lms24 merged commit 23a7d0b into master Jan 4, 2023

Lms24 deleted the lms-replay-debounce branch January 4, 2023 08:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

ref(replay): Replace `lodash.debounce` with custom debounce implementation#6593
Lms24 merged 7 commits intomasterfrom
lms-replay-debounce

Lms24 commented Dec 21, 2022 •

edited

Loading

Uh oh!

Uh oh!

github-actions bot commented Dec 21, 2022 •

edited

Loading

Uh oh!

lforst commented Dec 21, 2022

Uh oh!

Lms24 commented Dec 21, 2022 •

edited

Loading

Uh oh!

Lms24 left a comment

Uh oh!

Lms24 Dec 22, 2022

Uh oh!

billyvg Dec 22, 2022

Uh oh!

Lms24 Dec 22, 2022

Uh oh!

Lms24 Dec 22, 2022 •

edited

Loading

Uh oh!

Lms24 Dec 22, 2022

Uh oh!

mydea left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		// lodash.debounce is a CJS module, so we need to convert it to ESM first
		commonjs(),

Uh oh!

Conversation

Lms24 commented Dec 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Dec 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

lforst commented Dec 21, 2022

Uh oh!

Lms24 commented Dec 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Lms24 left a comment

Choose a reason for hiding this comment

Uh oh!

Lms24 Dec 22, 2022

Choose a reason for hiding this comment

Uh oh!

billyvg Dec 22, 2022

Choose a reason for hiding this comment

Uh oh!

Lms24 Dec 22, 2022

Choose a reason for hiding this comment

Uh oh!

Lms24 Dec 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lms24 Dec 22, 2022

Choose a reason for hiding this comment

Uh oh!

mydea left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Lms24 commented Dec 21, 2022 •

edited

Loading

github-actions bot commented Dec 21, 2022 •

edited

Loading

Lms24 commented Dec 21, 2022 •

edited

Loading

Lms24 Dec 22, 2022 •

edited

Loading