-
-
Notifications
You must be signed in to change notification settings - Fork 277
ref: Hub passes the Scope #114
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
Changes from 16 commits
8c6727d
7570846
c1033b2
c3f453a
a46e144
8a14620
4c3f323
35e3072
30fa7a9
9088c88
e93f85e
7b05b0b
1f70e4f
1f4bb44
d4334a9
d02947f
ba721bd
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 |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| import 'dart:async'; | ||
|
|
||
| import 'client.dart'; | ||
| import 'hub.dart'; | ||
| import 'protocol/event.dart'; | ||
| import 'protocol/level.dart'; | ||
| import 'protocol/sentry_id.dart'; | ||
|
|
||
| class NoOpHub implements Hub { | ||
| NoOpHub._(); | ||
|
|
||
| static final NoOpHub _instance = NoOpHub._(); | ||
|
|
||
| factory NoOpHub() { | ||
| return _instance; | ||
| } | ||
|
|
||
| @override | ||
| void bindClient(SentryClient client) {} | ||
|
|
||
| @override | ||
| Future<SentryId> captureEvent(Event event) => Future.value(SentryId.empty()); | ||
|
|
||
| @override | ||
| Future<SentryId> captureException(throwable, {stackTrace}) => | ||
| Future.value(SentryId.empty()); | ||
|
|
||
| @override | ||
| Future<SentryId> captureMessage( | ||
| String message, { | ||
| SeverityLevel level = SeverityLevel.info, | ||
| String template, | ||
| List params, | ||
| }) => | ||
| Future.value(SentryId.empty()); | ||
|
|
||
| @override | ||
| Hub clone() => this; | ||
|
|
||
| @override | ||
| void close() {} | ||
|
|
||
| @override | ||
| void configureScope(callback) {} | ||
|
|
||
| @override | ||
| bool get isEnabled => false; | ||
|
|
||
| @override | ||
| SentryId get lastEventId => SentryId.empty(); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,16 @@ | ||
| /// Sentry response id | ||
| class SentryId { | ||
| static const String emptyId = '00000000-0000-0000-0000-000000000000'; | ||
| static const String _emptyId = '00000000-0000-0000-0000-000000000000'; | ||
|
|
||
| /// The ID Sentry.io assigned to the submitted event for future reference. | ||
| final String _id; | ||
|
|
||
| String get id => _id; | ||
| // TODO: should we generate the new UUID here with an empty ctor? | ||
|
Contributor
Author
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. if so, we'd need to import the
Contributor
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. I maybe missed it, but I didn't found a method to generate a Uuid from String in the uuid package
Contributor
Author
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.
Contributor
Author
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. I created a task for it |
||
|
|
||
| const SentryId(this._id); | ||
|
|
||
| factory SentryId.empty() => SentryId(emptyId); | ||
| factory SentryId.empty() => SentryId(_emptyId); | ||
|
|
||
| @override | ||
| String toString() => _id.replaceAll('-', ''); | ||
|
|
||
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.
@rxlabz I noticed that all the methods are sync now because of
awaitshould we aim for sync calls or keep it async?
@bruno-garcia
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.
While we send the envelope to the client/transport to be sent to the network directly, this should be async.
We could make it sync if we, inside the client, pass the envelope to an Isolate. And there we could use async to send it to the network or write to disk.
I'd propose we stick to async for now, user code already expects do await on capture methods, and allows us to interface with the native layer by purely writing an envelope to disk (which is async)
if you're not using
async/awaiton the method body, just remove the keywords andreturn futureObj;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.
I'm not sure I understand. They are not sync, they return a Future
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.
@rxlabz
item.client.captureEventreturns a future, but you call this method usingawaitif you use the
awaitkeyboard, means you the future to be completed, so its a sync call, not async.or I am not understanding the
awaitkeyword in Dart, which is also possibleUh oh!
There was an error while loading. Please reload this page.
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.
awaitis just a "syntactic sugar", it doesn't change the nature of the code. Maybe https://dart.dev/codelabs/async-await will be clearer than me.But I noticed that this method is asynchronous when it calls item.client.captureEvent and synchronous if the hub is not enabled or if the event is null, we probably should change the signature to
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.
not a big fan because of:
but I got it I guess:
await item.client.captureEventmakes a sync call, but the signature of the methodhub.captureEventis alsoasyncso it's fine.let's keep as it is till we have a concrete implementation of the transport layer, so we'd know what looks better.