Skip to content

fix(browser): Add replay and profiling options to BrowserClientOptions #8921

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

Merged
merged 1 commit into from
Aug 31, 2023

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Aug 31, 2023

This PR adds missing replay and profiling options to the BrowserClientOptions interface. Previously, we only exposed these types to BrowserOptions. This led to type errors for users who directly create a client, without using Sentry.init (as reported in #8857).

While one would think that BrowserClientOptions is basically a superset of BrowserOptions (leaving aside stackparser, integrations and transport) this is in fact not the case, which IMO is the core problem here. BrowserClientOptions only inherits the base Options (which are shared between browser and node), in addition to transport options. However, changing this so that, BrowserClientOptions inherits from BrowserOptions is a breaking change, so I opted to just add the missing options to BrowserClientOptions.

Happy to change this if someone has an idea how to properly fix this without a breaking change.

Furthermore, I'm not sure how to best test this. We could simply add a unit test but having such a test without assertions feels a little awkward to me. Integration tests would probably be the better choice but ours aren't type checked.
I'll leave it untested for the time being (afaik we don't explicitly test types) but if reviewers prefer a test, I'm happy to come up with something.

closes #8857

@Lms24 Lms24 requested review from mydea and AbhiPrasad August 31, 2023 08:51
Copy link
Member

@mydea mydea left a comment

Choose a reason for hiding this comment

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

this seems good enough to me, let's :shipit:

@Lms24 Lms24 enabled auto-merge (squash) August 31, 2023 11:48
@Lms24 Lms24 merged commit 1d64a06 into develop Aug 31, 2023
@Lms24 Lms24 deleted the lms/fix-browser-client-options-replay-profiling branch August 31, 2023 11:54
Sign up for free to 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.

BrowserClientOptions type should include BrowserClientReplayOptions
2 participants