fix(loader): Improve loader callback handling

[mydea]

This slightly refactors the loader script to hopefully fix getsentry/sentry-javascript#9216.

• We used to pass onLoadCallbacks around, which is unnecessary since we are actually using the same instance everywhere that we mutate, so we can just access the variable directly.
• For the linked issue, I could replicate that sometimes SDK.init() was called before the onLoad handler was registered. I suspect this may be due to caching behavior where addEventListener('load') may fire immediately (??) - I could replicate it sometimes, sometimes not, when refreshing the page repeatedly. Anyhow, since we rely on this firing after a user had the chance to call Sentry.onLoad, I think it at least doesn't hurt to call this in the next event tick, to potentially fix racing conditions there.

[github-actions]

🚨 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]

I have a slight feeling that we're only contributing to the confusingness of this file with this change. Ideally, we should defer the calling of init until the script tag's load event handler fired no? I won't block this though.

[codecov]

Codecov Report

Merging #58070 (17a1f7d) into master (00fb8dd) will decrease coverage by 0.02%.
Report is 5 commits behind head on master.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #58070      +/-   ##
==========================================
- Coverage   79.11%   79.10%   -0.02%     
==========================================
  Files        5144     5143       -1     
  Lines      224118   224107      -11     
  Branches    37741    37740       -1     
==========================================
- Hits       177305   177273      -32     
- Misses      41136    41154      +18     
- Partials     5677     5680       +3     

Files	Coverage Δ
...ews/settings/organizationGeneralSettings/index.tsx	90.90% <ø> (ø)

... and 7 files with indirect coverage changes

[mydea]

I have a slight feeling that we're only contributing to the confusingness of this file with this change. Ideally, we should defer the calling of init until the script tag's load event handler fired no? I won't block this though.

I do not disagree, however it is sadly a bit opaque when the load event for the script tag triggers etc 😬 If we can think of a better way to do this, I'd be happy!!

[mydea]

I extracted all of this into a function & adjusted it so that we only call setupSDK in the next tick, which hopefully makes all of this a bit clearer!

[mydea]

Adding once and passive here which IMHO make sense (the handler can be cleaned up after being called), and in older browsers will just be ignored.

[mydea]

Renaming this to be a bit clearer, naming wise.

[mydea]

I think we can get rid of this. Instead, when a user call Sentry.forceLoad(), we simply inject the SDK the same way we would for a non-lazy setup - all the other logic is not needed IMHO?

[lforst]

Problem: Inbetween removing these and waiting for the next tick there might be an error which is not recorded. We need to remove these listeners right before we attach the actual non-buffered once, so in theory right before the call to the actual Sentry.init.

As discussed: Let's move it into the monkeypatch init.

[mydea]

moved this!

[mydea]

@lforst updated this according to what we talked about!