Skip to content

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

Merged
Lms24 merged 7 commits into
masterfrom
lms-replay-debounce
Jan 4, 2023
Merged

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

Conversation

@Lms24

@Lms24Lms24 commented Dec 21, 2022

Copy link
Copy Markdown
Member

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

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

Comment threadpackages/utils/src/debounce.ts Outdated
@github-actions

github-actionsBot commented Dec 21, 2022

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
@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

Copy link
Copy Markdown
Contributor

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

Lms24 commented Dec 21, 2022

Copy link
Copy Markdown
MemberAuthor

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
Lms24force-pushed the lms-replay-debounce branch from 6739731 to b7ae986CompareDecember 22, 2022 12:52

@Lms24Lms24 left a comment

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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.


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

Copy link
Copy Markdown
MemberAuthor

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
Copy Markdown
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
Copy Markdown
MemberAuthor

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

@Lms24Lms24Dec 22, 2022

Copy link
Copy Markdown
MemberAuthor

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

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

Copy link
Copy Markdown
MemberAuthor

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.

@Lms24
Lms24force-pushed the lms-replay-debounce branch from 964a080 to 8b0bc87CompareDecember 22, 2022 13:08

@mydeamydea left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great work! 🚀

@Lms24
Lms24 merged commit 23a7d0b into masterJan 4, 2023
@Lms24
Lms24 deleted the lms-replay-debounce branch January 4, 2023 08:17
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Replay] Remove lodash.debounce dependency

4 participants

@Lms24@lforst@billyvg@mydea