Support for new DSN format (without secretKey) and remove the quiver dependency. #20
Conversation
|
@yjbanov ping! |
Fixed dartfmt issues.
|
@tvolkert any chance of getting this merged in. I've a release to do and would like project referencing the official repo rather than the fork. |
|
Hi! Sorry, was out on vacation. Looking at this now. |
Me too - @slightfoot you caught the team in vacation season 😄 (leaving the review to @yjbanov) |
lib/sentry.dart
Outdated
| @@ -20,6 +19,9 @@ import 'src/version.dart'; | |||
|
|
|||
| export 'src/version.dart'; | |||
|
|
|||
| /// Used to provide time-stamp for logging. | |||
There was a problem hiding this comment.
nit: according to Wikipedia "timestamp" is the correct spelling 🤓
lib/sentry.dart
Outdated
| @@ -56,31 +58,27 @@ class SentryClient { | |||
| Event environmentAttributes, | |||
| bool compressPayload, | |||
| Client httpClient, | |||
| Clock clock, | |||
| ClockProvider clock, | |||
There was a problem hiding this comment.
To give people some time to migrate let's make clock a dynamic parameter. Then in the body of the factory we can do this:
final ClockProvider clockProvider = clock is ClockProvider
? clock
: clock.get;
// pass clockProvider to SentryClient._This still lets us remove our dependency on quiver but won't break existing code. Let's thoroughly document it in the dartdocs though.
| String authHeader = 'Sentry sentry_version=6, sentry_client=$sentryClient, ' | ||
| 'sentry_timestamp=${now.millisecondsSinceEpoch}, sentry_key=$publicKey'; | ||
| if (secretKey != null) { | ||
| authHeader += ', sentry_secret=$secretKey'; |
There was a problem hiding this comment.
Let's add a unit-test for this case.
There was a problem hiding this comment.
Added unit test for parsing and http header.
|
Thanks! The fewer dependencies the better! |
|
Thanks for the review @yjbanov I'll look into the details and answer when I get time to make the suggested changes. |
|
Adding @tvolkert to the reviewer list as I'll be on vacation again. |
| @@ -46,7 +48,9 @@ class SentryClient { | |||
| /// make HTTP calls to Sentry.io. This is useful in tests. | |||
| /// | |||
| /// If [clock] is provided, it is used to get time instead of the system | |||
| /// clock. This is useful in tests. | |||
| /// clock. This is useful in tests. Should be an implementation of ClockProvider. | |||
There was a problem hiding this comment.
References to in-scope identifiers should be wrapped in square brackets so that dartdoc can create links to them, in this case [ClockProvider].
I've updated the library to make the secretKey field optional so that it supports the new DSN format from Sentry.
I also noticed that Quiver is only used for Clock to hold a ticking DateTime for reporting. Looking at the use of it I see it can be replaced with a simple function typedef. This I feel is a better way to make the library for flexible and not have the extra dependency on the Quiver library.