-
-
Notifications
You must be signed in to change notification settings - Fork 257
Set addFeatureFlag
value parameter to dynamic and ignore non-boolean values
#2849
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2849 +/- ##
==========================================
- Coverage 87.78% 87.76% -0.03%
==========================================
Files 269 269
Lines 8970 8971 +1
==========================================
- Hits 7874 7873 -1
- Misses 1096 1098 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
iOS Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
1b0c8a3 | 1259.30 ms | 1282.78 ms | 23.48 ms |
6a3cb89 | 1244.43 ms | 1262.86 ms | 18.43 ms |
9d43f71 | 1225.06 ms | 1227.06 ms | 2.00 ms |
6e083bb | 1244.33 ms | 1264.60 ms | 20.26 ms |
62dde43 | 1258.43 ms | 1276.81 ms | 18.38 ms |
136c365 | 1248.12 ms | 1277.33 ms | 29.20 ms |
0095354 | 1241.96 ms | 1269.25 ms | 27.29 ms |
77db8d4 | 1228.47 ms | 1248.80 ms | 20.33 ms |
c70e01a | 1273.00 ms | 1299.12 ms | 26.12 ms |
e0f6628 | 1250.57 ms | 1274.86 ms | 24.29 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
1b0c8a3 | 8.38 MiB | 9.75 MiB | 1.37 MiB |
6a3cb89 | 8.42 MiB | 9.89 MiB | 1.47 MiB |
9d43f71 | 8.29 MiB | 9.39 MiB | 1.10 MiB |
6e083bb | 8.16 MiB | 9.17 MiB | 1.01 MiB |
62dde43 | 8.16 MiB | 9.17 MiB | 1.01 MiB |
136c365 | 8.38 MiB | 9.75 MiB | 1.37 MiB |
0095354 | 8.38 MiB | 9.78 MiB | 1.40 MiB |
77db8d4 | 8.38 MiB | 9.73 MiB | 1.35 MiB |
c70e01a | 8.16 MiB | 9.17 MiB | 1.01 MiB |
e0f6628 | 8.32 MiB | 9.50 MiB | 1.18 MiB |
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.
Just one question.
@@ -360,7 +360,11 @@ class Sentry { | |||
/// Gets the current active transaction or span bound to the scope. | |||
static ISentrySpan? getSpan() => _hub.getSpan(); | |||
|
|||
static Future<void> addFeatureFlag(String name, bool value) async { | |||
static Future<void> addFeatureFlag(String name, dynamic value) async { |
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.
If we allow dynamic users could set whole objects, or would we check for string
, int
type etc?
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.
we will check for the type in the future, this is how JS does it as well https://github.com/getsentry/sentry-javascript/blob/56aac537ea2627baec9025291b73f46b48670abf/packages/browser/src/utils/featureFlags.ts#L59C1-L66C1
the specs allow for multiple types even though we only accept booleans currently.
This will be advantageous in the future when we eventually allow more types.
#skip-changelog