Skip to content

Conversation

mydea
Copy link
Member

@mydea mydea commented Jan 19, 2023

Since we now have a default flush time === max flush time in replay, we can skip the duplicate timer in that case.

@mydea mydea added the Package: replay Issues related to the Sentry Replay SDK label Jan 19, 2023
@mydea mydea requested a review from Lms24 January 19, 2023 12:07
@mydea mydea self-assigned this Jan 19, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jan 19, 2023

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.82 KB (+0.01% 🔺)
@sentry/browser - ES5 CDN Bundle (minified) 61.47 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.5 KB (+0.02% 🔺)
@sentry/browser - ES6 CDN Bundle (minified) 54.77 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 20.21 KB (0%)
@sentry/browser - Webpack (minified) 66.17 KB (0%)
@sentry/react - Webpack (gzipped + minified) 20.24 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 47.48 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.74 KB (+0.01% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 25.03 KB (+0.02% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 43.27 KB (-0.81% 🔽)
@sentry/replay - Webpack (gzipped + minified) 38.05 KB (-1% 🔽)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 60.54 KB (-0.58% 🔽)

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Nice improvement. I think we already test equal max/min waits so I think we're good test-wise.

In the future we might be able to simplify the debounce function further, if we decide that maxwait should always equal wait. But let's leave it for the moment as-is

@mydea
Copy link
Member Author

mydea commented Jan 19, 2023

Nice improvement. I think we already test equal max/min waits so I think we're good test-wise.

In the future we might be able to simplify the debounce function further, if we decide that maxwait should always equal wait. But let's leave it for the moment as-is

Jup, my thoughts as well!

@mydea mydea enabled auto-merge (squash) January 19, 2023 12:39
@mydea mydea force-pushed the fn/avoid-debounce-duplicate-timers branch from 015ea3c to 84f889e Compare January 19, 2023 13:04
});

it('uploads a replay event if 15 seconds have elapsed since the last replay upload', async () => {
it('uploads a replay event if maxFlushDelay is set 15 seconds have elapsed since the last replay upload', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, any clue why this test wasn't failing before? 😅 I suspect there is actually a bug (??) that is fixed here, but not sure - we do have a test for the wait time being equal, but maybe it doesn't catch some aspect...?!

expect(replay.eventBuffer?.pendingLength).toBe(0);
});

it('uploads a replay event if 5 seconds have elapsed since the last replay event occurred', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

this test was actually here twice!

expect(replay.eventBuffer?.pendingLength).toBe(0);
});

it('uploads a replay event if 15 seconds have elapsed since the last replay upload', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Same for this test, exact duplicate 😅

@mydea mydea merged commit 29c651d into master Jan 19, 2023
@mydea mydea deleted the fn/avoid-debounce-duplicate-timers branch January 19, 2023 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: replay Issues related to the Sentry Replay SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants