Skip to content
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

fix(loader): Do not preprocess unhandled promise event in loader #60077

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

mydea
Copy link
Member

@mydea mydea commented Nov 16, 2023

This is actually handled in the SDK, so we don't have to double process this. And the processing in the loader is error prone, because we do not guard for what p could be.

I checked and even in v6 this was already checked in the SDK (e.g. see here from 2 years ago: https://github.com/getsentry/sentry-javascript/blob/4cd71c5792453d16acd0137af7534fe9345abd93/packages/browser/src/integrations/globalhandlers.ts)

Also saving a few bytes ;)

Closes getsentry/sentry-javascript#9579

Test in SDK added here: getsentry/sentry-javascript#9581

This is actually handled in the SDK, so we don't have to double process this.

I checked and even in v6 this was already checked in the SDK (e.g. see here from 2 years ago: https://github.com/getsentry/sentry-javascript/blob/4cd71c5792453d16acd0137af7534fe9345abd93/packages/browser/src/integrations/globalhandlers.ts)
@mydea mydea requested review from lforst and Lms24 November 16, 2023 09:19
@mydea mydea self-assigned this Nov 16, 2023
@github-actions github-actions bot added Scope: Frontend Automatically applied to PRs that change frontend components Scope: Backend Automatically applied to PRs that change backend components labels Nov 16, 2023
Copy link
Contributor

🚨 Warning: This pull request contains Frontend and Backend changes!

It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently.

Have questions? Please ask in the #discuss-dev-infra channel.

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.

Good change!

Copy link

codecov bot commented Nov 16, 2023

Codecov Report

Merging #60077 (c06219d) into master (ba674cd) will decrease coverage by 0.01%.
Report is 2 commits behind head on master.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #60077      +/-   ##
==========================================
- Coverage   80.76%   80.76%   -0.01%     
==========================================
  Files        5179     5179              
  Lines      227417   227417              
  Branches    38256    38256              
==========================================
- Hits       183679   183672       -7     
- Misses      38144    38152       +8     
+ Partials     5594     5593       -1     

see 5 files with indirect coverage changes

@mydea mydea merged commit a1d8b20 into master Nov 16, 2023
55 checks passed
@mydea mydea deleted the fn/loader-promise-check branch November 16, 2023 12:40
mydea added a commit to getsentry/sentry-javascript that referenced this pull request Nov 16, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unhandled exception handler can potentially crash
3 participants