-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(replay): Replace _waitForError
with recordingMode
#6489
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
Conversation
_waitForError
with mode
_waitForError
with mode
size-limit report 📦
|
There was a problem hiding this 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.
There was a problem hiding this 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!
64e5fe3
to
75e0900
Compare
👍 I changed it to |
_waitForError
with mode
_waitForError
with recordingMode
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 besession
orerror
. I think this describes better what is going on, and makes it a bit more explicit when using this etc.