Skip to content

Conversation

mydea
Copy link
Member

@mydea mydea commented Dec 9, 2022

After checking what could be going wrong in #6466, one possible culprit is that we mangle properties prefixed with _. Meaning that it may break when referencing such properties from outside.

As far as I see, we do this in one place as of now - we access _waitForError. So this PR refactors this.

However, imho for a "public" API (of this class, at least), it is not a great API to have waitForError, as it is a bit abstract what that means.
So I changed this to instead be mode, which can be session or error. I think this describes better what is going on, and makes it a bit more explicit when using this etc.

@mydea mydea added the Package: replay Issues related to the Sentry Replay SDK label Dec 9, 2022
@mydea mydea requested review from billyvg and Lms24 December 9, 2022 15:41
@mydea mydea self-assigned this Dec 9, 2022
@mydea mydea changed the title fix(replay): Replay _waitForError with mode fix(replay): Replace _waitForError with mode Dec 9, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.68 KB (-0.01% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 60.96 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.47 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified) 54.51 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 20.27 KB (0%)
@sentry/browser - Webpack (minified) 66.25 KB (0%)
@sentry/react - Webpack (gzipped + minified) 20.29 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 47.29 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.67 KB (-0.01% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 25.12 KB (+0.01% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 41.83 KB (+0.1% 🔺)
@sentry/replay - Webpack (gzipped + minified) 37.89 KB (+0.03% 🔺)

Copy link
Member

@billyvg billyvg left a comment

Choose a reason for hiding this comment

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

Maybe recordMode to be more specific?

And ensure that `mode` is public, so it isn't mangled.
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.

l: Agree w/ Billy that recordingMode is probably a little more verbose.

Let's get this in so that we get rid of the private field. This isn't everything we have to do to fix minified CDN bundles but definitely a cleaner solution than just ignoring the private field in our terser config. Great catch!

@mydea mydea force-pushed the fn/replay-avoid-use-private branch from 64e5fe3 to 75e0900 Compare December 12, 2022 08:25
@mydea
Copy link
Member Author

mydea commented Dec 12, 2022

👍 I changed it to recordingMode!

@mydea mydea changed the title fix(replay): Replace _waitForError with mode fix(replay): Replace _waitForError with recordingMode Dec 12, 2022
@mydea mydea merged commit 497a3c7 into master Dec 12, 2022
@mydea mydea deleted the fn/replay-avoid-use-private branch December 12, 2022 08:46
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.

3 participants