-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
ref(replay): Make Session a POJO #6417
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
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.
Nice!
Eventually, we'd like to get rid of nullish coalescencing operators to save some bundle size but we can tackle this when we get to bundle size optimization later on as well. |
Good point, I'll implement that as well while I'm at it! Check it out. |
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.
Awesome! Feels like v7 work all over again, we right back to it!
/** | ||
* Get a session with defaults & applied sampling. | ||
*/ | ||
export function makeSession(session: Partial<Session>, { sessionSampleRate, errorSampleRate }: SampleRates): Session { |
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.
nit: If we get rid of the obj arg in favour of direct args with sessionSampleRate
and errorSampleRate
we'll save some bytes because we can minify those variables.
This makes sense to do if makeSession
is not user facing, as then we can change the method signature in the future.
So I adjusted this a bit based on feedback from @AbhiPrasad . basically, I extracted the sampling decision out into a dedicated function, that only needs to be called when creating a new session. No need to actually call this in |
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.
LGTM. I like the sampled
change
As this is only used in tests, we don't need to include this in the user-facing bundle.
To trim down bundle size a bit.
430e6fe
to
a0c0afd
Compare
While the replay session was made a class here: getsentry/sentry-replay#91, since we landed #6315, it's not really necessary anymore to have this be a class.
This PR refactors this to a simple POJO with a function to easily generate it with defaults etc.
Also, some things like the
toJSON()
where only used in tests. We should generally avoid this, and instead provide some test utils specifically for these things (where necessary), to avoid shipping this to our end users.