Improve custom Loader Script init flow

[mydea]

Currently, when using the Loader Script, you have to configure it like this:

<script src="... loader URL"></script>
<script>
Sentry.onLoad(function() {
  Sentry.init({ ... });
});
</script>

In order for this to work, we depend on very specific timing semantics - you have to call Sentry.onLoad after you added the loader script, but before the loader script fetched the CDN bundle.

In order to streamline this, I propose to add a new way to do this:

<script>
window.sentryOnLoad = function() {
  Sentry.init({ ... });
};
</script>

This could be configured either before or after the loader script tag, and we would pick it up in the loader, if configured, in addition to any Sentry.onLoad() callbacks.

Alternatively, we could also make it an array:

<script>
window.sentryOnLoad = window.sentryOnLoad || [];
window.sentryOnLoad.push(function() {
  Sentry.init({ ... });
});
</script>

This has the benefit of allowing multiple calls, but makes the setup a bit more complicated. I would personally vote for the first option, for simplicity - if you need multiple callbacks, you can still use Sentry.onLoad().

[lforst]

The google analytics is strong in this one :D

[asottile-sentry]

is there a plan to deprecate the "old" way? I'd rather not see two official ways to do something when one already works

[mydea]

so the "old" way still has a purpose, as you can use it to guard usage of APIs. E.g. anything that is not handled by the loader, you can guard:

// Safe to use, because this is handled by the loader
Sentry.captureException(); 

// This is NOT safe to use, because this may not be loaded/available yet
Sentry.startTransaction();

// Instead, you have to do:
Sentry.onLoad(function() {
  Sentry.startTransaction();
});

This new API is specifically desgined to streamline the custom init flow for the loader! onLoad is a bit more versatile and can be called multiple times.

[asottile-sentry]

I'm worried that this feels rushed -- especially when there's a correct way to do it already and that we would need to attach yet-another thing to window. Is there a broader design document and agreement that this is the correct way forward?

[mydea]

The problem is that the current way is not necessarily correct for what it is trying to achieve:

1. You need to guard for the existence of Sentry.onLoad, because it could be unexisting if e.g. the loader script failed to load.
2. We depend on fragile timing semantics to ensure that any init called inside of Sentry.onLoad() is actually applied correctly before other things. See e.g. Loader script not reliably setting beforeNavigate context name sentry-javascript#9216

[asottile-sentry]

can you please put together a design / rfc

[HazAT]

is there a plan to deprecate the "old" way? I'd rather not see two official ways to do something when one already works

We can never deprecate it since we have no influence over which customers use the old version of the code so it has to exist forever.

Tho with the changes @mydea I am not sure if we still need this?

[mydea]

Sadly it seems the issue has not been fixed - it was also more of a bandaid, sadly, as the root cause is tricky. The actual solution is to define your custom init before the loader script is loaded, then we can ensure this is correct. See also the comment here: getsentry/sentry-javascript#9216 (comment) who basically asks for exactly this.

From a docs perspective, I would only recommend this going forward for custom initialization, but we would of course (as @HazAT pointed out) continue to support Sentry.onLoad indefinitely, and can document it as a way to guard custom API calling going forward.

[mydea]

Since we don't have another real fix for the timing issues, we're going to go ahead with this change in a backwards compatible way.