-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Conversation
d05c14e
to
d65055e
Compare
There was a problem hiding this 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.
There was a problem hiding this 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
aeb4922
to
e3e14ba
Compare
size-limit report 📦
|
0612a03
to
b321096
Compare
@@ -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; |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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']; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 👍🏻
b321096
to
ec37e37
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! 🚀
There was a problem hiding this 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` : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l: just for clarity
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) |
There was a problem hiding this comment.
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']; |
There was a problem hiding this comment.
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?
d171f49
to
108f53f
Compare
Move relevant integrations. Part of #9833