Skip to content

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 Lms24 and lforst 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
@github-actions
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!

@codecov
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
@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

4 participants