Analytics: Refactor EventTracker producers so they are always singletons #11948
Conversation
|
@zhouyx Ready for review. |
There was a problem hiding this comment.
Can we also migrate to the getTrackerForOptions in test-events.js
I see places where we create tracker manually. https://github.com/ampproject/amphtml/blob/master/extensions/amp-analytics/0.1/test/test-events.js#L1076
| * @param {!Object<string, function(new:./events.EventTracker)>} options | ||
| * @return {?./events.EventTracker} | ||
| */ | ||
| getTrackerForOptions(name, options) { |
There was a problem hiding this comment.
The option here is serving as a klass whitlist. would whitelistKlass be a better name?
| */ | ||
| getTrackerForOptions(name, options) { | ||
| const trackerProfile = options[name]; | ||
| if (!!trackerProfile) { |
There was a problem hiding this comment.
I don't think we need !! here.
There was a problem hiding this comment.
Done. It explicitly casts to a boolean value, which if blocks do implicitly, so it's mostly stylistic here.
| @@ -450,7 +450,7 @@ export class AnalyticsGroup { | |||
| if (!trackerProfile && !isEnumValue(AnalyticsEventType, eventType)) { | |||
| trackerProfile = EVENT_TRACKERS['custom']; | |||
| } | |||
| if (trackerProfile) { | |||
| if (!!trackerProfile) { | |||
There was a problem hiding this comment.
why we need !! here? I believe if (trackerProfile) is good enough.
| @@ -142,6 +142,21 @@ export class AnalyticsRoot { | |||
| getElementById(unusedId) {} | |||
|
|
|||
| /** | |||
| * Returns the tracker for the specified name and list of allowed types. | |||
There was a problem hiding this comment.
can we add a unit test here.
| new SUPPORT_WAITFOR_TRACKERS[waitForSpec](this.root); | ||
| let waitForTracker = this.waitForTrackers_[waitForSpec]; | ||
| if (!waitForTracker) { | ||
| waitForTracker = this.root.getTrackerForOptions( |
There was a problem hiding this comment.
root.getTrackerForOptions() can return a null, it's safer to check the value before calling waitForTracker.getElementSignal.
I know this should never happen here because we've checked SUPPORT_WAITFOR_TRACKERS[waitForSpec] before. But still. Maybe it would be better to move around the check.
|
@pomeroyr just one more request. please see |
|
@zhouyx Done. I chose to just use getTracker since every single use in test expects only one possible result anyway. |
To resolve test errors.
|
Thanks for the refactor!! |
…ons (ampproject#11948) * analytics root produces eventtrackers * test and rename * fix lint * force tests * force tests * no more one off trackers in test * force tests * force tests
…ons (ampproject#11948) * analytics root produces eventtrackers * test and rename * fix lint * force tests * force tests * no more one off trackers in test * force tests * force tests
Related to #11813, refactor which allows the dynamic production of EventTrackers inside EventTrackers without producing one-off objects.