fix(browser): Ensure lazyLoadIntegration works in NPM mode
#11673
Conversation
size-limit report 📦
|
| @@ -33,12 +33,15 @@ const WindowWithMaybeIntegration = WINDOW as { | |||
| export async function lazyLoadIntegration(name: keyof typeof LazyLoadableIntegrations): Promise<IntegrationFn> { | |||
| const bundle = LazyLoadableIntegrations[name]; | |||
|
|
|||
| if (!bundle || !WindowWithMaybeIntegration.Sentry) { | |||
| // `window.Sentry` is only set when using a CDN bundle, but this method can also be used via the NPM package | |||
There was a problem hiding this comment.
Maybe I'm missing some context here but since we never set window.Sentry in NPM mode, should we really start doing it now? Just wondering since we have the __SENTRY__ carrier object anyway 🤔
There was a problem hiding this comment.
The problem is that the integration CDN bundles always add to window.Sentry, so we can't really influence this here (unless we change the logic in our CDN bundles, which is also possible of course 🤔 )
There was a problem hiding this comment.
Yeah I see the dilemma 😅
unless we change the logic in our CDN bundles, which is also possible of course
I wouldn't do that - CDN users definitely expect window.Sentry APIs and probably also integrations being available this way.
So I guess the lesser of the two evils is to also create a window.Sentry object in NPM mode. I still have the feeling that this might bite us into the back but we can worry about that if it comes up. I guess the one concrete concern would be users who create a window.Sentry object themselves. I think I've seen this before in some unsupported SSR framework but I didn't find the issue/discussion that I had in mind. And even in this case, we only add a property to the object and wouldn't overwrite it if it was there previously. So 🤞
There was a problem hiding this comment.
Yeah, but even then we'd put the integration onto that object, which is not ideal but should generally also not break stuff (hopefully xD)
| throw new Error(`Cannot lazy load integration: ${name}`); | ||
| } | ||
|
|
||
| // Bail if the integration already exists | ||
| const existing = WindowWithMaybeIntegration.Sentry[name]; | ||
| const existing = sentryOnWindow[name]; |
There was a problem hiding this comment.
related to my comment above: Assuming we already installed the integration in NPM mode during the normal Sentry.init call, it wouldn't have been registered on window.Sentry so we wouldn't actually bail here.
Again, I feel like I lack some context so feel free to disregard!
There was a problem hiding this comment.
So this case specifically I thought about too, my idea was that we can/should check for this outside of this method - my reasoning was that otherwise we'd have to also pass the integration name (e.g. HttpClient ) to the function because we'd need this to lookup via getIntegrationByName() 🤔 No strong feelings, we can also extend the function signature and check in here?
There was a problem hiding this comment.
cc @ryan953 also relates to your comment. So to clarify we have to options IMHO:
- Keep this function "lean" so it does not check integration name - this will keep the bundle size slightly smaller, and instead at call site you have to do something like this:
function maybeLoadFeedback() {
if(client.getIntegrationByName('Feedback')) {
return;
}
return lazyLoadIntegration('feedbackIntegration');
}- Do the check in the integration: For this we have to options, either requiring to pass in both function name as well as integration name, or adding this to the map somehow. The latter has a nicer DX, but will increase bundle size:
2.a: lazyLoadIntegration('feedbackIntegration', 'Feedback') <-- smaller bundle size, but not-so-nice usage
2.b: lazyLoadIntegration('feedbackIntegration') and keep feedbackIntegration > Feedback in the map somehow
For me, the main reason I went with 1. was that if a user uses this, they probably don't need this check (think in the future we want users to use this when using the loader to add pluggable integrations), and thus we can keep the API surface & bundle size slightly smaller. But I'm also totally fine with adding this additional check.
There was a problem hiding this comment.
I like 1. for what it's worth.
In the case of an npm bundle the user could easily have feedbackIntegration or something included in the bundle, but not added at runtime, therefore getIntegrationByName would not return and we'd still inject a script tag to load the integration again. For this reason I think it makes sense to avoid the extra check inside the method.
There was a problem hiding this comment.
Ok yes, that makes sense, let's go with 1 then. Thx for explaining!
mydea
force-pushed
the
fn/lazyLoadIntegrationNpm
branch
from
0625084 to
11f8d45
Compare
|
In terms of It looks like this method is very much a "given a valid integration name: 1. ensure a script tag is on the page to load it 2. return the integration to the callsite" |
As noticed by @ryan953 here: #11621 (comment), this was not actually working properly in NPM-mode. I added a proper test for this and fixed this, so hopefully should be all good now.