Skip to content
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

Export SpotlightBrowser Sentry integration #403

Closed
2 tasks
Lms24 opened this issue May 31, 2024 · 7 comments
Closed
2 tasks

Export SpotlightBrowser Sentry integration #403

Lms24 opened this issue May 31, 2024 · 7 comments
Assignees
Labels

Comments

@Lms24
Copy link
Member

Lms24 commented May 31, 2024

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

@Lms24 Lms24 added this to the 2.0 milestone May 31, 2024
@Shubhdeep12
Copy link
Collaborator

Shubhdeep12 commented Jun 1, 2024

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 Lms24 removed this from the 2.0 milestone Jun 13, 2024
@Lms24
Copy link
Member Author

Lms24 commented Jun 13, 2024

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
Copy link
Member Author

Lms24 commented Jun 13, 2024

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
Copy link
Collaborator

Shubhdeep12 commented Jun 13, 2024

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
Copy link
Member Author

Lms24 commented Jun 13, 2024

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!

@BYK
Copy link
Member

BYK commented Aug 7, 2024

Being done in getsentry/sentry-javascript#13263

@BYK
Copy link
Member

BYK commented Aug 30, 2024

This is now merged and released. Closing the ticket as we don't intend to export the built-in integration yet.

One thing to watch out for is a potential clash between the SDK-side integration and the built-in integration: #511.

@BYK BYK closed this as completed Aug 30, 2024
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants