-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
chore: Refactor event tracking method #11262
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
e4e57fc
to
6c90712
Compare
6c90712
to
7f37836
Compare
With @NicolasMassart we had a call and aligned on the API this Event Builder can have, here are some examples of the expected behavior: // Event Builder interface
const eventBuilder = createEventBuilder(EVENT_NAME)
.addProperties({ ...})
.addSensitiveProperties({ ... })
.addProperties({ ... }) // These can be added multiple times
if (condition) {
eventBuilder.addProperties({ ... })
eventBuilder.removeProperties(['prop1', 'prop2'])
// or inline
// eventBuilder.addProperties({ ... }).removeProperties(['prop1', 'prop2'])
}
const event = eventBuilder.build();
// Suggested way to track this event
// We pass the built event to the trackEvent function
trackEvent(event);
// or inline
trackEvent(createEventBuilder(EVENT).addProperties({ ... }).build());
// And backwards compatible (for now?? deprecate it in the future).
trackEvent({ name: EVENT, properties: { ... }, sensitiveProperties: { ... } });
// What if I want to modify an already built event?
// We can revert it to a Builder state if we create a new builder from the event
createEventBuilder(event).addProperties({ ... }).build();
// ———
// Another way (initial suggestion) was to have a track method in the event object
// THIS WILL NOT BE SUPPORTED
event.track(); If there's a use case we detect, in the future the builder can be extended to support new features, for example: eventBuilder.makeSensitive(['prop1', 'prop2']);
eventBuilder.makeNonSensitive(['prop1', 'prop2']); |
encapsulate track function overload
4233171
to
5bb3300
Compare
Bitrise❌❌❌ Commit hash: 0c964b4 Note
Tip
|
…tamask-mobile into 10784_simplify_metrics_calls
Bitrise✅✅✅ Commit hash: 0aa75cf Note
|
Quality Gate passedIssues Measures |
Bitrise✅✅✅ Commit hash: 9b72ec5 Note
|
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.
LGTM! Thanks for those explanations.
Fyi I am not a codeowner on this, so looks like you'll still need to get that as well.
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.
Looks good to me
Description
Note
These changes are made in order to preserve existing mechanisms in parallel.
This leads to strange naming like ITrackingEvent as the ideal IMetaMetricsEvent already exists.
This can be dealt with later as it's well documented and following guidelines.
Related issues
fixes #11319
Manual testing steps
NA
Screenshots/Recordings
NA
Before
NA
After
NA
Pre-merge author checklist
Pre-merge reviewer checklist