-
-
Notifications
You must be signed in to change notification settings - Fork 240
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
feat(js-sdk): use js transport #2563
base: feat/js-sdk-integration
Are you sure you want to change the base?
Changes from 15 commits
1742be6
9492f48
b85a672
2d468e0
fbc0a98
ce8a0b7
141a180
0621a51
9601a40
53e71cd
a26ffc9
490d0a1
68fb10e
a148380
3e66ae6
affee90
a558d55
d8b31ef
1c8202f
d4009c1
1b55a05
4d121ce
bfcac4d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -123,7 +123,12 @@ mixin SentryFlutter { | |
// Not all platforms have a native integration. | ||
if (_native != null) { | ||
if (_native!.supportsCaptureEnvelope) { | ||
options.transport = FileSystemTransport(_native!, options); | ||
// Sentry's native web integration is only enabled when enableSentryJs=true. | ||
// Transport configuration happens in web_integration because the configuration | ||
// options aren't available until after the options callback executes. | ||
if (!options.platformChecker.isWeb) { | ||
options.transport = FileSystemTransport(_native!, options); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it make sense to set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's not possible here since this is happening within I added the comment above that, maybe the comment can be improved to clarify this better There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this will be possible for v9 where we use the JS SDK by default |
||
} | ||
if (!options.platformChecker.isWeb) { | ||
options.addScopeObserver(NativeScopeObserver(_native!)); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
import '../../sentry_flutter.dart'; | ||
import '../native/sentry_native_binding.dart'; | ||
|
||
class JavascriptTransport implements Transport { | ||
JavascriptTransport(this._binding, this._options); | ||
|
||
final SentryFlutterOptions _options; | ||
final SentryNativeBinding _binding; | ||
|
||
@override | ||
Future<SentryId?> send(SentryEnvelope envelope) async { | ||
try { | ||
await _binding.captureEnvelopeObject(envelope); | ||
} catch (exception, stackTrace) { | ||
_options.logger( | ||
SentryLevel.error, | ||
'Failed to send envelope', | ||
exception: exception, | ||
stackTrace: stackTrace, | ||
); | ||
return Future.value(SentryId.empty()); | ||
} | ||
|
||
return envelope.header.eventId; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a particular reason why it's called
captureEnvelopeObject
instead of justcaptureEnvelope
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
captureEnvelope already exists which only takes
Uint8List
and dart sadly doesnt support method overloading, we could theoretically use captureEnvelope but then we would have to convert it back to an envelope object which is unnecessary so that's why I added another captureEnvelope that is supposed to use the actual envelope object and not any other data structureThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other options is to just add another parameter
SentryEnvelope envelope
to the existing captureEnvelope but imo I'd prefer to have a second function since we dont have union types in dart, maybe in Dart 3 with sealed classes