-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(integrations): Export pluggable integrations directly #8842
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
(statement: { specifiers: [{ imported?: { name: string }; name?: string }] }, source: string) => { | ||
// We only want to handle the integrations import if it doesn't come from the @sentry/browser re-export | ||
// In that case, we just want to leave it alone | ||
if ( |
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.
Without this change, this fails for all tests as the build output of webpack includes the import from @sentry/integrations
by default and tries to "fix" it in there, but it also does not contain an imported
field so it just fails.
@@ -29,7 +29,6 @@ | |||
"tslib": "^2.4.1 || ^1.9.3" | |||
}, | |||
"devDependencies": { | |||
"@sentry/browser": "7.64.0", |
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.
This would have been a circular dependency, so got rid of this (similar to what we had to do for replay)
size-limit report 📦
|
@@ -16,6 +17,7 @@ const INTEGRATIONS = { | |||
...windowIntegrations, | |||
...CoreIntegrations, | |||
...BrowserIntegrations, | |||
...PluggableIntegrations, |
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.
h: This will not tree-shake as soon as people import Integrations
(and not even access one of the pluggable ones) because the spread operator (or in fact assigning an object) is a runtime operation. It's for the same reason we have BrowserTracing
as a direct export and not under Integrations
.
We need to do the same here - exporting the pluggable integrations top level.
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.
right, that makes sense of course... I wonder actually if we should then completely get rid of the Integrations
key and move all of these to the top level instead? 🤔 And then maybe suffix them with Integration
or something like this...:
export { Dedupe as DedupeIntegration } from '@sentry/integrations';
// etc
IMHO it would be a bit weird to have the default integrations be available under Integrations
and the pluggable ones directly at the root 😅
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.
In the next major we should remove the Integrations object for sure. I agree having them in separate places is weird. I think we could export everything top level.
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.
This PR re-exports all pluggable integrations from
@sentry/integrations
from the browser/node packages.This mean you can use them like this:
We already depend on treeshaking in a bunch of other places, and this change means there is really nothing you'd ever need to install manually in addition to your SDK package, also ensuring we generally always have consistent versions etc.