Refactor BindingUtils to enable the use of custom bindings
#1184
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## v7.0.0 #1184 +/- ##
=========================================
Coverage ? 87.97%
=========================================
Files ? 120
Lines ? 3784
Branches ? 0
=========================================
Hits ? 3329
Misses ? 455
Partials ? 0 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
@ueman I will review that after the holidays, thank you for doing this btw. |
| if (binding == null) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
I believe this should stay, otherwise, it won't work anyway, so bail out early.
| late SentryWidgetsBindingObserver _observer; | ||
| late SentryFlutterOptions _options; |
There was a problem hiding this comment.
Better to keep it nullable, so if close is called before call which is wrong, wouldn't throw, it's defensive programming against API misusing.
| @@ -26,9 +26,11 @@ | |||
| - theme | |||
| - Removed isolate name from Dart context. It's now reported via the threads interface. It can be enabled via `options.attachThreads` | |||
| - Use `sentryClientName` instead of `sdk.identifier` ([#1135](https://github.com/getsentry/sentry-dart/pull/1135)) | |||
| - Refactor `BindingUtils` to `BindingWrapper` to enable the use of custom bindings ([#1184](https://github.com/getsentry/sentry-dart/pull/1184)) | |||
There was a problem hiding this comment.
If this is a refactor, we don't need an entry here, it's more about user-facing changes.
There was a problem hiding this comment.
I intend this to be user facing, although maybe it shouldn't be advertised.
flutter/test/integrations/not_initialized_widgets_binding_test.dart
Outdated
Show resolved
Hide resolved
flutter/test/integrations/on_error_integration_no_binding_test.dart
Outdated
Show resolved
Hide resolved
Co-authored-by: Manoel Aranda Neto <[email protected]>
📜 Description
This change enables the use of a custom WidgetsBinding. It doesn't introduce a custom binding though.
This change is done for the v7 branch, since this could be considered a breaking change.
💡 Motivation and Context
I'm experimenting with initializing a custom widgets binding to add even more observability. Currently, that's not really supported. That's why I propose the following refactoring.
The new interface is modelled after the methods of the
WidgetsBindingSentry needs. By using a custom implementation of theBindingWrapperand assigning it to the options, you can make use of a custom binding. The custom binding then gets correctly initialized and used by Sentry.💚 How did you test it?
This is covered by existing tests.
📝 Checklist
🔮 Next steps