Skip to content

Allow to capture replays for Electron when node integration is enabled #6630

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

Closed
timfish opened this issue Dec 30, 2022 · 2 comments · Fixed by #6644
Closed

Allow to capture replays for Electron when node integration is enabled #6630

timfish opened this issue Dec 30, 2022 · 2 comments · Fixed by #6644
Assignees
Labels
Package: replay Issues related to the Sentry Replay SDK

Comments

@timfish
Copy link
Collaborator

timfish commented Dec 30, 2022

Which package are you using?

@sentry/electron

SDK Version

master

Steps to Reproduce

Ref: getsentry/sentry-electron#606 (comment)

This code:

setupOnce(): void {
if (!isBrowser()) {
return;
}

Means that Replay cannot be used in Electron renderers when nodeIntegration is enabled as process is defined. This has not been the default config for a couple of years and it's not recommended but it's still used a lot in real world applications.

Expected Result

Replay should work in Electron renderers

Actual Result

Doesn't work

Is this check required? What is it trying to protect against?

@timfish timfish added Type: Question Package: replay Issues related to the Sentry Replay SDK labels Dec 30, 2022
@mydea
Copy link
Member

mydea commented Jan 3, 2023

Basically, the idea of this check is to ensure we do not run replay when running in non-browser environments, e.g. node (especially e.g. Next.js or similar envs). As replay relies on a lot of DOM etc. stuff.

I would be a bit hesistant to change this as it could lead to a bunch of other errors. We could also just check for existence of window, which may be good enough, but may also let some other stuff slip through I guess.

Options I see, cc @billyvg & @Lms24:

  1. Change nothing, Electron apps with process will not work with Replay
  2. Relax the check to only check for existence of window
  3. Allow to overwrite this in an experiment, e.g. _experiments.isBrowser = true or similar

@Lms24
Copy link
Member

Lms24 commented Jan 3, 2023

I agree that we should be careful with making changes here.

@timfish Is there away we can add a check for electron specifically?

So that in the end we could do something along the lines of

if (!isBrowser() && !isElectronWithProcess()) return

@timfish timfish self-assigned this Jan 3, 2023
@mydea mydea changed the title isBrowser checks stop replay from starting in Electron renderers when node integration is enabled Allow to capture replays for Electron when node integration is enabled Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: replay Issues related to the Sentry Replay SDK
Projects
None yet
3 participants