Skip to content

Conversation

@mydea
Copy link
Member

@mydea mydea commented Oct 23, 2023

Allow to configure a new window.sentryOnLoad callback, which can be defined before the Loader Script is loaded, thus allowing to streamline timing issues & prevent issues if e.g. the loader script was not loaded.

tests added here getsentry/sentry-javascript#9331

Closes #58428

@mydea mydea self-assigned this Oct 23, 2023
@mydea mydea requested a review from a team as a code owner October 23, 2023 09:31
@mydea mydea requested review from Lms24 and lforst October 23, 2023 09:32
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Oct 23, 2023
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Oct 23, 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.

@lforst
Copy link
Contributor

lforst commented Oct 23, 2023

I think it might make sense to make sentryOnLoad an array of callbacks because you might want to add callbacks in many different places in your code?

@mydea
Copy link
Member Author

mydea commented Oct 23, 2023

Really this should only be used to call Sentry.init(), which you should only do once in your code. because otherwise you will/may again have race conditions, think:

window.sentryOnLoad = window.sentryOnLoad || [];
window.sentryOnLoad.push(function() {
  Sentry.captureException('xxx');
});

// somewhere else
window.sentryOnLoad = window.sentryOnLoad || [];
window.sentryOnLoad.push(function() {
  Sentry.init(customConfig);
});

You'll have no way to ensure that the correct init is called.

IMHO for such scenarios it would be better to:

  1. define your custom init only in window.sentryOnLoad
  2. Use Sentry.onLoad for the other stuff, as we can then ensure this is called after window.sentryOnLoad (you'll still have to guard in this case for window.Sentry && window.Sentry.onLoad(), but IMHO this is fine, as this is more of an edge case - generally you should stick to the loader public APIs when using the loader anyhow 😅

@codecov
Copy link

codecov bot commented Oct 23, 2023

Codecov Report

Merging #58574 (0b46d7b) into master (71890e0) will decrease coverage by 0.01%.
Report is 5 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #58574      +/-   ##
==========================================
- Coverage   80.67%   80.67%   -0.01%     
==========================================
  Files        5160     5160              
  Lines      226364   226359       -5     
  Branches    38139    38138       -1     
==========================================
- Hits       182624   182607      -17     
- Misses      38163    38174      +11     
- Partials     5577     5578       +1     
Files Coverage Δ
...try/api/endpoints/organization_release_assemble.py 84.78% <100.00%> (-1.50%) ⬇️
static/app/components/sidebar/index.tsx 91.66% <ø> (ø)
...views/alerts/rules/metric/triggers/chart/index.tsx 81.03% <ø> (ø)
static/app/views/alerts/rules/metric/types.tsx 100.00% <ø> (ø)

... and 6 files with indirect coverage changes

Copy link
Contributor

@lforst lforst left a comment

Choose a reason for hiding this comment

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

convinced!

@mydea mydea merged commit 2c57b8a into master Nov 7, 2023
@mydea mydea deleted the fn/loader-onload branch November 7, 2023 08:56
mydea added a commit to getsentry/sentry-javascript that referenced this pull request Nov 7, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Nov 22, 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.

Improve custom Loader Script init flow

3 participants