Skip to content

fix(cdn): Fix SDK source for CDN bundles #13475

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.

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 9 commits into from
Aug 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .size-limit.js
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ module.exports = [
path: createCDNPath('bundle.tracing.min.js'),
gzip: false,
brotli: false,
limit: '111 KB',
limit: '113 KB',
},
{
name: 'CDN Bundle (incl. Tracing, Replay) - uncompressed',
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { expect } from '@playwright/test';
import { SDK_VERSION } from '@sentry/browser';

import { sentryTest } from '../../../../utils/fixtures';
import { envelopeRequestParser, waitForErrorRequestOnUrl } from '../../../../utils/helpers';
Expand All @@ -11,3 +12,22 @@ sentryTest('captureException works', async ({ getLocalTestUrl, page }) => {

expect(eventData.message).toBe('Test exception');
});

sentryTest('should capture correct SDK metadata', async ({ getLocalTestUrl, page }) => {
const url = await getLocalTestUrl({ testDir: __dirname });
const req = await waitForErrorRequestOnUrl(page, url);

const eventData = envelopeRequestParser(req);

expect(eventData.sdk).toMatchObject({
name: 'sentry.javascript.browser',
version: SDK_VERSION,
integrations: expect.any(Object),
packages: [
{
name: 'loader:@sentry/browser',
version: SDK_VERSION,
},
],
});
});
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
import { expect } from '@playwright/test';
import type { Event } from '@sentry/types';
import { SDK_VERSION } from '@sentry/browser';

import { sentryTest } from '../../../../utils/fixtures';
import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers';
import { envelopeRequestParser, waitForErrorRequestOnUrl } from '../../../../utils/helpers';

sentryTest('should capture a simple error with message', async ({ getLocalTestPath, page }) => {
const url = await getLocalTestPath({ testDir: __dirname });

const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);
sentryTest('should capture a simple error with message', async ({ getLocalTestUrl, page }) => {
const url = await getLocalTestUrl({ testDir: __dirname });
const req = await waitForErrorRequestOnUrl(page, url);
const eventData = envelopeRequestParser(req);

expect(eventData.exception?.values).toHaveLength(1);
expect(eventData.exception?.values?.[0]).toMatchObject({
Expand All @@ -22,3 +22,23 @@ sentryTest('should capture a simple error with message', async ({ getLocalTestPa
},
});
});

sentryTest('should capture correct SDK metadata', async ({ getLocalTestUrl, page }) => {
const isCdn = (process.env.PW_BUNDLE || '').startsWith('bundle');

const url = await getLocalTestUrl({ testDir: __dirname });
const req = await waitForErrorRequestOnUrl(page, url);
const eventData = envelopeRequestParser(req);

expect(eventData.sdk).toEqual({
name: 'sentry.javascript.browser',
version: SDK_VERSION,
integrations: expect.any(Object),
packages: [
{
name: `${isCdn ? 'cdn' : 'npm'}:@sentry/browser`,
version: SDK_VERSION,
},
],
});
});
3 changes: 0 additions & 3 deletions dev-packages/rollup-utils/npmHelpers.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import {
makeImportMetaUrlReplacePlugin,
makeNodeResolvePlugin,
makeRrwebBuildPlugin,
makeSetSDKSourcePlugin,
makeSucrasePlugin,
} from './plugins/index.mjs';
import { makePackageNodeEsm } from './plugins/make-esm-plugin.mjs';
Expand All @@ -45,7 +44,6 @@ export function makeBaseNPMConfig(options = {}) {
const importMetaUrlReplacePlugin = makeImportMetaUrlReplacePlugin();
const cleanupPlugin = makeCleanupPlugin();
const extractPolyfillsPlugin = makeExtractPolyfillsPlugin();
const setSdkSourcePlugin = makeSetSDKSourcePlugin('npm');
const rrwebBuildPlugin = makeRrwebBuildPlugin({
excludeShadowDom: undefined,
excludeIframe: undefined,
Expand Down Expand Up @@ -106,7 +104,6 @@ export function makeBaseNPMConfig(options = {}) {

plugins: [
nodeResolvePlugin,
setSdkSourcePlugin,
sucrasePlugin,
debugBuildStatementReplacePlugin,
importMetaUrlReplacePlugin,
Expand Down
3 changes: 2 additions & 1 deletion dev-packages/rollup-utils/plugins/bundlePlugins.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,9 @@ export function makeIsDebugBuildPlugin(includeDebugging) {
export function makeSetSDKSourcePlugin(sdkSource) {
return replace({
preventAssignment: false,
delimiters: ['', ''],
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 with this change, we should now have no real bundle size impact in either scenario. We simply do not overwrite stuff at all by default, returning npm. But for CDN bundles, we replace the comment, not the value itself, which is correctly minified to what we want.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that without the delimiter part, it does not correctly replace the string, for whatever reason.

values: {
__SENTRY_SDK_SOURCE__: JSON.stringify(sdkSource),
'/* __SENTRY_SDK_SOURCE__ */': `return ${JSON.stringify(sdkSource)};`,
},
});
}
Expand Down
4 changes: 2 additions & 2 deletions packages/utils/src/env.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,6 @@ export function isBrowserBundle(): boolean {
* Get source of SDK.
*/
export function getSDKSource(): SdkSource {
// @ts-expect-error __SENTRY_SDK_SOURCE__ is injected by rollup during build process
return __SENTRY_SDK_SOURCE__;
// This comment is used to identify this line in the CDN bundle build step and replace this with "return 'cdn';"
/* __SENTRY_SDK_SOURCE__ */ return 'npm';
}
Loading