feat: Add spotlightBrowser integration #13263
Conversation
| // don't support the `spotlight` option there and rely on the users | ||
| // adding the `spotlightBrowserIntegration()` to their integrations which | ||
| // wouldn't get initialized with the check below when there's no DSN set. | ||
| this._options.integrations.some(({ name }) => name.startsWith('Spotlight')) |
There was a problem hiding this comment.
I think this is fine given spotlight is pretty much a one-off at the moment. Another option would be to adjust our integrations API to acomodate for such scenarios, say in an afterSdkInit hook that's aways invoked, regardless if the SDK is enabled or not. Will bring this up with the team but let's not block the PR on this as it's easily changeable.
There was a problem hiding this comment.
Agreed. I thought about adding a special property like forced or something like that on the integration but I'll leave that debate to you folks
packages/node/src/sdk/index.ts
Outdated
| @@ -140,13 +140,19 @@ function _init( | |||
| const scope = getCurrentScope(); | |||
| scope.update(options.initialScope); | |||
|
|
|||
| if (options.spotlight && options.integrations.some(({ name }) => name !== SPOTLIGHT_INTEGRATION_NAME)) { | |||
There was a problem hiding this comment.
m: My brain is a bit fried already today but the the .some check isn't correct right? We only want to add the integration if it hasn't been added to options.integrations before. I think we can use .find instead
There was a problem hiding this comment.
You are right, I got confused! Fix coming out that said I prefer .some() to .find() as we don't really need the first found value, we just want to check if there is anything that matches the criteria. They probably work the same underneath but they don't have as you can optimize .some() in different ways if you wanted to.
| if (isEnabled(client)) { | ||
| client.init(); | ||
| } | ||
| client.init(); |
There was a problem hiding this comment.
I was a bit scared when I saw this but as far as I can tell we don't extend the init from BaseClient in the derived client classes. Given we guard the base client init implementation this should be fine.
There was a problem hiding this comment.
Event if we extend, aren't we supposed to call super()? I think that's the responsibility of downstream class implementors and not something we should be worried about here?
| @@ -140,7 +140,7 @@ function _init( | |||
| const scope = getCurrentScope(); | |||
| scope.update(options.initialScope); | |||
|
|
|||
| if (options.spotlight && options.integrations.some(({ name }) => name !== SPOTLIGHT_INTEGRATION_NAME)) { | |||
| if (options.spotlight && !options.integrations.some(({ name }) => name === SPOTLIGHT_INTEGRATION_NAME)) { | |||
There was a problem hiding this comment.
We can also do
| if (options.spotlight && !options.integrations.some(({ name }) => name === SPOTLIGHT_INTEGRATION_NAME)) { | |
| if (options.spotlight && options.integrations.every(({ name }) => name !== SPOTLIGHT_INTEGRATION_NAME)) { |
But I think the current version expresses the intent a bit more clearly: if there are no integrations matching this name, add it.
There was a problem hiding this comment.
I'm fine with any of these, as long as we only add the integration if it isn't yet present 👍
Thanks for fixing!
| @@ -140,7 +140,7 @@ function _init( | |||
| const scope = getCurrentScope(); | |||
| scope.update(options.initialScope); | |||
|
|
|||
| if (options.spotlight && options.integrations.some(({ name }) => name !== SPOTLIGHT_INTEGRATION_NAME)) { | |||
| if (options.spotlight && !options.integrations.some(({ name }) => name === SPOTLIGHT_INTEGRATION_NAME)) { | |||
There was a problem hiding this comment.
I'm fine with any of these, as long as we only add the integration if it isn't yet present 👍
Thanks for fixing!
…ser package Follow up to #13263 where we forgot to export it from the main package 🤦
No description provided.