Skip to content

feat(v8/integrations): Merge integrations into core #10799

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 11 commits into from
Feb 27, 2024

Conversation

s1gr1d
Copy link
Member

@s1gr1d s1gr1d commented Feb 23, 2024

Move relevant integrations. Part of #9833

@s1gr1d s1gr1d requested review from lforst and Lms24 February 23, 2024 13:31
@s1gr1d s1gr1d force-pushed the sig-merge-integrations-to-core branch from d05c14e to d65055e Compare February 23, 2024 13:40
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Given HttpClient and ReportingObserver integrations are going straight into @sentry/browser, how about we move all bundling logic from core to browser package?

So packages/core has no bundle build (and no rollup.bundle.config.mjs), and instead we just add new bundles to browser workflow.

This means that with the removal of @sentry/integrations we consolidate all the CDN building logic to one central package.

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

I agree, creating all integration CDN bundles from browser seems like a good idea to me too.

I suggest we do this in a couple of steps:

  • move "universal" integrations into core in this PR (what you already did in this PR minus the CDN bundle changes)
  • move browser-only integrations into browser (I think you already got CDN bundles working for them so feel free to include them in the PR or in a separate one, whatever you prefer)
  • add missing integration CDN bundles in browser package

@s1gr1d s1gr1d force-pushed the sig-merge-integrations-to-core branch from aeb4922 to e3e14ba Compare February 26, 2024 12:34
@s1gr1d s1gr1d requested review from AbhiPrasad and Lms24 February 26, 2024 12:35
@s1gr1d s1gr1d self-assigned this Feb 26, 2024
Copy link
Contributor

github-actions bot commented Feb 26, 2024

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay, Feedback) - Webpack (gzipped) 77.27 KB (-0.02% 🔽)
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 68.54 KB (-0.02% 🔽)
@sentry/browser (incl. Tracing, Replay with Canvas) - Webpack (gzipped) 72.45 KB (-0.02% 🔽)
@sentry/browser (incl. Tracing, Replay) - Webpack with treeshaking flags (gzipped) 62.08 KB (+0.04% 🔺)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 32.7 KB (-0.08% 🔽)
@sentry/browser (incl. browserTracingIntegration) - Webpack (gzipped) 32.7 KB (-0.08% 🔽)
@sentry/browser (incl. Feedback) - Webpack (gzipped) 30.91 KB (-0.02% 🔽)
@sentry/browser (incl. sendFeedback) - Webpack (gzipped) 30.91 KB (-0.02% 🔽)
@sentry/browser - Webpack (gzipped) 22.18 KB (-0.08% 🔽)
@sentry/browser (incl. Tracing, Replay, Feedback) - ES6 CDN Bundle (gzipped) 75.69 KB (-0.01% 🔽)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped) 67.39 KB (-0.1% 🔽)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped) 33.17 KB (-0.2% 🔽)
@sentry/browser - ES6 CDN Bundle (gzipped) 24.72 KB (-0.06% 🔽)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed) 211 KB (-0.02% 🔽)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed) 99.72 KB (-0.05% 🔽)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed) 73.92 KB (-0.06% 🔽)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped) 36.26 KB (-0.07% 🔽)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 68.79 KB (-0.06% 🔽)
@sentry/react - Webpack (gzipped) 22.19 KB (-0.12% 🔽)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 85.23 KB (-0.02% 🔽)
@sentry/nextjs Client - Webpack (gzipped) 49.57 KB (-0.12% 🔽)
@sentry-internal/feedback - Webpack (gzipped) 17.14 KB (0%)

@s1gr1d s1gr1d force-pushed the sig-merge-integrations-to-core branch from 0612a03 to b321096 Compare February 27, 2024 13:10
@@ -129,14 +129,17 @@ function generateSentryAlias(): Record<string, string> {

return Object.fromEntries(
packageNames.map(packageName => {
// pluggable integrations exist in the browser package
const actPackageName = packageName === 'integrations' ? 'browser' : packageName;
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be removed again - there is no more package with that name I believe?

@@ -59,7 +59,7 @@ export function makeBaseBundleConfig(options) {
plugins: [rrwebBuildPlugin, markAsBrowserBuildPlugin],
};

// used by `@sentry/integrations` and `@sentry/wasm` (bundles which need to be combined with a stand-alone SDK bundle)
// used by `@sentry/wasm` (bundles which need to be combined with a stand-alone SDK bundle)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// used by `@sentry/wasm` (bundles which need to be combined with a stand-alone SDK bundle)
// used by `@sentry/wasm` & pluggable integrations from core/browser (bundles which need to be combined with a stand-alone SDK bundle)

this is a bit more accurate I guess?

@@ -8,6 +8,10 @@ if (targets.some(target => target !== 'es5' && target !== 'es6')) {
throw new Error('JS_VERSION must be either "es5" or "es6"');
}

const addonIntegrationFiles = ['contextlines', 'httpclient', 'reportingobserver'];
Copy link
Member

Choose a reason for hiding this comment

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

let's call this browserPluggableIntegrationFiles maybe? A bit more explicit what it does, wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, fits better with coreIntegrationFiles below 👍🏻

@s1gr1d s1gr1d force-pushed the sig-merge-integrations-to-core branch from b321096 to ec37e37 Compare February 27, 2024 15:13
Copy link
Member

@mydea mydea left a comment

Choose a reason for hiding this comment

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

Great work! 🚀

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Awesome!

MIGRATION.md Outdated

We moved optional integrations from their own package (`@sentry/integrations`) to `@sentry/browser` and `@sentry/node`.

Integrations that are now exported from `@sentry/browser` :
Copy link
Member

Choose a reason for hiding this comment

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

l: just for clarity

Suggested change
Integrations that are now exported from `@sentry/browser` :
Integrations that are now exported from `@sentry/browser` (or framework-specific packages like `@sentry/react`):

@@ -59,7 +59,7 @@ export function makeBaseBundleConfig(options) {
plugins: [rrwebBuildPlugin, markAsBrowserBuildPlugin],
};

// used by `@sentry/integrations` and `@sentry/wasm` (bundles which need to be combined with a stand-alone SDK bundle)
// used by `@sentry/wasm` & pluggable integrations from core/browser (bundles which need to be combined with a stand-alone SDK bundle)
Copy link
Member

Choose a reason for hiding this comment

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

(not relevant for this PR): This reminds me, we should also move @sentry/wasm into browser. It's also just an integration. So now that we can build and publish integration CDN bundles in browser, I think it should be fairly straight forward to bring it in.

@@ -8,6 +8,10 @@ if (targets.some(target => target !== 'es5' && target !== 'es6')) {
throw new Error('JS_VERSION must be either "es5" or "es6"');
}

const browserPluggableIntegrationFiles = ['contextlines', 'httpclient', 'reportingobserver'];

const coreIntegrationFiles = ['captureconsole', 'debug', 'dedupe', 'extraerrordata', 'rewriteframes', 'sessiontiming'];
Copy link
Member

Choose a reason for hiding this comment

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

super-l: wdyt about naming this corePluggableIntegrationFiles for consistency?

@s1gr1d s1gr1d force-pushed the sig-merge-integrations-to-core branch from d171f49 to 108f53f Compare February 27, 2024 16:01
@s1gr1d s1gr1d enabled auto-merge (squash) February 27, 2024 16:01
@s1gr1d s1gr1d merged commit a5c4fcb into develop Feb 27, 2024
@s1gr1d s1gr1d deleted the sig-merge-integrations-to-core branch February 27, 2024 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants