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

fix(electron): Do not sync to NativeClient when autoDetectErrors or nativeCrashes are disabled #2040

Merged
merged 7 commits into from Nov 20, 2023

Conversation

yousif-bugsnag
Copy link
Contributor

@yousif-bugsnag yousif-bugsnag commented Nov 15, 2023

Goal

If either of enabledErrorTypes.nativeCrashes or autoDetectErrors are disabled, we don't install the sync layer (NativeClient.install) but still call all the methods to sync state to the native client.

Currently this logs an error message: Sync layer is not installed, first call install(), which isn't very useful as users can't call NativeClient.install

This PR updates each of the plugins that sync to the native layer to ensure that they do not sync to the native layer if those config options are disabled. This affects:

  • plugin-electron-client-state-persistence
  • plugin-electron-app
  • plugin-electron-device
  • plugin-electron-session

Testing

Added unit tests to assert that the native sync methods are not called when the relevant config options are disabled.

Also updated the makeClientForPlugin function in electron-test-helpers to support creating a client with an array of plugins - this allows the client-state-persistence unit tests to make use of the test helper since the plugin depends on the client-state-manager plugin being loaded.

Copy link

github-actions bot commented Nov 15, 2023

@bugsnag/browser bundle size diff

Minified Minfied + Gzipped
Before 43.67 kB 13.40 kB
After 43.67 kB 13.40 kB
± No change No change

code coverage diff

<temporarily disabled>

Generated by 🚫 dangerJS against 81080c9

Copy link
Member

@imjoehaines imjoehaines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be safer to check for enabledErrorTypes.nativeCrashes before trying to sync to the native client rather than removing this error

@yousif-bugsnag yousif-bugsnag force-pushed the PLAT-11169/electron-sync-layer-log branch from 7a51cff to d37abc8 Compare November 20, 2023 13:58
@yousif-bugsnag yousif-bugsnag changed the title Remove Electron "sync layer is not installed" error log fix(electron): Do not sync to NativeClient when autoDetectErrors or nativeCrashes are disabled Nov 20, 2023
@yousif-bugsnag yousif-bugsnag force-pushed the PLAT-11169/electron-sync-layer-log branch 2 times, most recently from b05739e to 2dbb794 Compare November 20, 2023 17:09
Copy link
Member

@gingerbenw gingerbenw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple of small comments, but approved based on unit tests as I don't understand enough about Session.prototype._track or client._sessionDelegate and why you would use one over the other.

@yousif-bugsnag yousif-bugsnag force-pushed the PLAT-11169/electron-sync-layer-log branch from 2dbb794 to 81080c9 Compare November 20, 2023 17:33
@yousif-bugsnag
Copy link
Contributor Author

couple of small comments, but approved based on unit tests as I don't understand enough about Session.prototype._track or client._sessionDelegate and why you would use one over the other.

Session.prototype._track just updates the handled/unhandled event count for the session, whereas client._sessionDelegate is responsible for starting/pausing/resuming sessions

@yousif-bugsnag yousif-bugsnag merged commit 836db68 into next Nov 20, 2023
68 checks passed
@yousif-bugsnag yousif-bugsnag deleted the PLAT-11169/electron-sync-layer-log branch November 20, 2023 18:40
@yousif-bugsnag yousif-bugsnag mentioned this pull request Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants