fix(loader): Ensure we add integrations#47082
Conversation
|
🚨 Warning: This pull request contains Frontend and Backend changes! It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently. Have questions? Please ask in the |
There was a problem hiding this comment.
integrations can be an function 😢 - we'll have to account for that use case as well.
There was a problem hiding this comment.
hmmm, good point! I would tend to ignore this case for now (=do not register default integrations if this is a function). WDYT? I think everything else would be a bit tricky, probably...
There was a problem hiding this comment.
yup i think its fine to ignore! it is pretty advanced config, so we can add a note in the docs about it.
There was a problem hiding this comment.
👍 updated this, also updating the script in sentry-javascript PR and adding a test for this here: getsentry/sentry-javascript@386cd2b
0e75d51 to
6b226e7
Compare
| } | ||
|
|
||
| // We want to ensure to only add default integrations if they haven't been added by the user. | ||
| function setupDefaultIntegrations(config: any, SDK: any) { |
There was a problem hiding this comment.
not a blocker but isn't there a better type that we could use here?
There was a problem hiding this comment.
IMHO We'd need to port all the types from JS-SDK into here to make this meaningful. It may be worthwhile to generally add more types here, but we'd need to start at the top there - I'd keep this for a follow up PR :)
|
|
||
| // We want to ensure to only add default integrations if they haven't been added by the user. | ||
| function setupDefaultIntegrations(config: any, SDK: any) { | ||
| const integrations: {name: string}[] = config.integrations || []; |
There was a problem hiding this comment.
| const integrations: {name: string}[] = config.integrations || []; | |
| const integrations: {name: string}[] = config.integrations ?? []; |
There was a problem hiding this comment.
This transpiles into a slightly more verbose form, which we generally try to avoid for ES5 code, as it adds a few - in this case unnecessary - bytes. In this case, I think || is fine. (Working on the JS SDK burns this into your brain xD)
I noticed one gap in our new loader implementation, namely that if you configure any integrations yourself, the defaults will not be added anymore. This PR fixes this by ensuring we add them only if they haven't been added by the user.
I noticed one gap in our new loader implementation, namely that if you configure any integrations yourself, the defaults will not be added anymore. This PR fixes this by ensuring we add them only if they haven't been added by the user.
I noticed one gap in our new loader implementation, namely that if you configure any integrations yourself, the defaults will not be added anymore. This PR fixes this by ensuring we add them only if they haven't been added by the user.
I noticed one gap in our new loader implementation, namely that if you configure any integrations yourself, the defaults will not be added anymore. This PR fixes this by ensuring we add them only if they haven't been added by the user.
I noticed one gap in our new loader implementation, namely that if you configure any integrations yourself, the defaults will not be added anymore.
This PR fixes this by ensuring we add them only if they haven't been added by the user.
Tests for this upcoming in sentry-javascript.