getsentry / spotlight

Your Universal Debug Toolbar
https://spotlightjs.com
Other
327 stars 11 forks source link

Export `SpotlightBrowser` Sentry integration #403

Open Lms24 opened 1 month ago

Lms24 commented 1 month ago

If users run spotlight standalone/with the electron app, the current automatic SpotlightBrowser injection logic won't work. I propose we export the spotlightBrowserIntegration from the @spotlightjs/overlay and @spotlightjs/spotlight packages so that users can register the integration themselves in their browser apps and don't have to rely on the auto injection mechanism.

Important: We'll keep auto injection as-is (meaning it's on by default but can be deactivated). This is just an additional way to get events from the browser into spotlight.

### Tasks
- [ ] Export `spotlightBrowserIntegration`
- [ ] Add documentation
Shubhdeep12 commented 1 month ago

hey @Lms24 i think here we are concerned of the scenario when user is using spotlight in app with browser sdk but dont want overalay on app and is using electron app.

how about instead of exporting sentryBrowserIntegration, we add a client attribute to current sentry integration(which is already exported from spotlight)?

User can send Sentry.client() in it and spotlight gets injected in that client, otherwise default way of injecting.

it will be something like -

Spotlight.init({
    debug: true,
    integrations: [sentry({ client: Sentry.getClient() })],
})

let me know if this sounds good to you, i'll push the code.

Lms24 commented 1 month ago

This would be a possibility but unfortunately, it only fixes half the problem. For example, take the Spotlight Electron app. If people build a frontend application but want to use Spotlight with the app instead of the built-in overlay, there is no overlay that's injected into the frontend application. Meaning we couldn't pass a client to the electron app. We can however export the spotlightBrowserIntegration from @spotlightjs/spotlight which users can then register in their Sentry.init function in the frontend application.

Does this sound reasonable to you?

Lms24 commented 1 month ago

Anyway, we can export the integration whenever we want; it's not a breaking change as long as we try to auto-inject the integration by default (which we do at the moment). I therefore removed it from the 2.0 milestone.

Shubhdeep12 commented 1 month ago

This would be a possibility but unfortunately, it only fixes half the problem. For example, take the Spotlight Electron app. If people build a frontend application but want to use Spotlight with the app instead of the built-in overlay, there is no overlay that's injected into the frontend application. Meaning we couldn't pass a client to the electron app. We can however export the spotlightBrowserIntegration from @spotlightjs/spotlight which users can then register in their Sentry.init function in the frontend application.

Does this sound reasonable to you?

Yup, this is good. I was in a thought that in both of the cases we have to import something from spotlight. So why not stick to init function we have where we will add some attributes to pass client(as shared in above example) OR We can add something like "useApp : boolean" which will not inject the overlay. Instead, just add spotlightbrowserintegration in sentry.

Lms24 commented 1 month ago

We can add something like "useApp : boolean" which will not inject the overlay. Instead, just add spotlightbrowserintegration in sentry.

Hmm yeah, maybe that's another angle. Let me think about it for a bit!