Skip to content

fix(browser): Ensure lazyLoadIntegration works in NPM mode #11673

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 23, 2024
Merged

fix(browser): Ensure lazyLoadIntegration works in NPM mode #11673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9776917
fix(browser): Ensure `lazyLoadIntegration` works in NPM mode
mydea Apr 18, 2024
11f8d45
fix test
mydea Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions ...ckages/browser-integration-tests/suites/integrations/lazyLoad/validIntegrationNpm/init.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import * as Sentry from '@sentry/browser';

// So we can use this in subject.js
// We specifically DO NOT set this on window.Sentry as we want to test a non-CDN bundle environment,
// where window.Sentry is usually not available
window._testSentry = { ...Sentry };

Sentry.init({
dsn: 'https://[email protected]/1337',
integrations: [],
});
7 changes: 7 additions & 0 deletions ...ges/browser-integration-tests/suites/integrations/lazyLoad/validIntegrationNpm/subject.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
window._testLazyLoadIntegration = async function run() {
const integration = await window._testSentry.lazyLoadIntegration('httpClientIntegration');

window._testSentry.getClient()?.addIntegration(integration());

window._integrationLoaded = true;
};
39 changes: 39 additions & 0 deletions ...ckages/browser-integration-tests/suites/integrations/lazyLoad/validIntegrationNpm/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import { expect } from '@playwright/test';
import { SDK_VERSION } from '@sentry/browser';

import { sentryTest } from '../../../../utils/fixtures';

sentryTest('it allows to lazy load an integration when using the NPM package', async ({ getLocalTestUrl, page }) => {
const bundle = process.env.PW_BUNDLE || '';
// We only want to run this in non-CDN bundle mode
if (bundle.startsWith('bundle')) {
sentryTest.skip();
}

const url = await getLocalTestUrl({ testDir: __dirname });

await page.route(`https://browser.sentry-cdn.com/${SDK_VERSION}/httpclient.min.js`, route => {
return route.fulfill({
status: 200,
contentType: 'application/javascript;',
body: "window.Sentry = window.Sentry || {};window.Sentry.httpClientIntegration = () => ({ name: 'HttpClient' })",
});
});

await page.goto(url);

const hasIntegration = await page.evaluate('!!window._testSentry.getClient().getIntegrationByName("HttpClient")');
expect(hasIntegration).toBe(false);

const scriptTagsBefore = await page.evaluate<number>('document.querySelectorAll("script").length');

await page.evaluate('window._testLazyLoadIntegration()');
await page.waitForFunction('window._integrationLoaded');

const scriptTagsAfter = await page.evaluate<number>('document.querySelectorAll("script").length');

const hasIntegration2 = await page.evaluate('!!window._testSentry.getClient().getIntegrationByName("HttpClient")');
expect(hasIntegration2).toBe(true);

expect(scriptTagsAfter).toBe(scriptTagsBefore + 1);
});
9 changes: 6 additions & 3 deletions packages/browser/src/utils/lazyLoadIntegration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

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 🤔 )

Copy link
Member

@Lms24 Lms24 Apr 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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 🤞

Copy link
Member Author

Choose a reason for hiding this comment

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

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)

const sentryOnWindow = (WindowWithMaybeIntegration.Sentry = WindowWithMaybeIntegration.Sentry || {});

if (!bundle) {
throw new Error(`Cannot lazy load integration: ${name}`);
}

// Bail if the integration already exists
const existing = WindowWithMaybeIntegration.Sentry[name];
const existing = sentryOnWindow[name];
Copy link
Member

Choose a reason for hiding this comment

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

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!

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

cc @ryan953 also relates to your comment. So to clarify we have to options IMHO:

  1. 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');
}
  1. 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.

Copy link
Member

@ryan953 ryan953 Apr 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Ok yes, that makes sense, let's go with 1 then. Thx for explaining!

if (typeof existing === 'function') {
return existing;
}
Expand All @@ -61,7 +64,7 @@ export async function lazyLoadIntegration(name: keyof typeof LazyLoadableIntegra
throw new Error(`Error when loading integration: ${name}`);
}

const integrationFn = WindowWithMaybeIntegration.Sentry[name];
const integrationFn = sentryOnWindow[name];

if (typeof integrationFn !== 'function') {
throw new Error(`Could not load integration: ${name}`);
Expand Down
6 changes: 0 additions & 6 deletions packages/browser/test/unit/utils/lazyLoadIntegration.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,6 @@ describe('lazyLoadIntegration', () => {
await expect(() => lazyLoadIntegration('invalid!!!')).rejects.toThrow('Cannot lazy load integration: invalid!!!');
});

test('it rejects without global Sentry variable', async () => {
await expect(() => lazyLoadIntegration('httpClientIntegration')).rejects.toThrow(
'Cannot lazy load integration: httpClientIntegration',
);
});

test('it does not inject a script tag if integration already exists', async () => {
// @ts-expect-error For testing sake
global.Sentry = Sentry;
Expand Down
Loading