-
Notifications
You must be signed in to change notification settings - Fork 54
Asserts added to check all relevant GA4 limitations #96
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
Asserts added to check all relevant GA4 limitations #96
Conversation
@bwilkerson this contains asserts that should enable us to find any events that are breaking GA4's limitations as we discussed last week. I suggest that we internally set the |
@@ -511,6 +532,8 @@ class TestAnalytics extends AnalyticsImpl { | |||
userProperty: userProperty, | |||
); | |||
|
|||
if (_enableAsserts) checkBody(body); |
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 don't understand the reasoning behind adding a separate flag when the VM already has a flag that serves the same purpose.
You could just as easily have removed the field _enableAsserts
, changed checkBody
to always return true
, and written
assert(validBody(body));
It wouldn't need a message because it will always explicitly throw an exception when the body
isn't valid.
Anyway, that seems much cleaner, and means that
- anyone can enable the checks when there's a need for it without having to write code that passes in the flag, and
- we can't ever accidentally leave the assert enabled in production code.
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.
Hm that's interesting, is this also the same behavior for the flutter tool? I was only made aware of this flag recently and didn't think that it was implemented for the flutter cli.
If i'm wrong, I'm happy to refactor this PR
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 don't know whether the flutter
CLI supports passing additional flags to the VM. It's quite possible that it doesn't, because I believe it's produced by an AOT compilation, so the asserts are probably already stripped out.
Are we planning to add flags to the dart
and flutter
commands to allow a different value of the flag to be passed it? If so, then we should leave it as is. If not, then it isn't clear that the current structure has value.
I guess I'm just too accustomed to running from source, or at least from a snapshot, where the flag can still be passed in at runtime.
As with all of my feedback, use you best judgement about whether to change the code or ignore the feedback.
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.
How we use this new flag will need to be discussed, the purpose of this PR was to have the code ready to be used in what we decide as a team.
But to continue using the --enable-asserts
flag from dart, we could set this flag in the Analytics
constructor to true if that flag was detected when run through the dart cli command... and on the flutter side, we can determine another approach since we don't have --enable-asserts
for the flutter cli
Merge master
Addresses:
Analytics.development(...)
constructor #94Asserts added (and can be turned on from factory constructors) to check against GA4 limitations. By default
Analytics()
constructor will have asserts disabled and theAnalytics.development()
constructor will have them enabledContribution guidelines:
dart format
.Many Dart repos have a weekly cadence for reviewing PRs - please allow for a week or two of latency for initial review feedback.