📈 AmpStoryEventTracker & Integration tests #24548
Conversation
Enriqe
changed the title
|
|
||
| * @param {function(!AnalyticsEvent)} listener | ||
| */ | ||
| fireListener_(event, rootTarget, config, listener) { |
There was a problem hiding this comment.
To make sure I understand: we're adding a "fireListener" method to be able to maybe not fire duplicate events depending on the configuration?
There was a problem hiding this comment.
Yep, that's right. This is going to make it easier for when we introduce the configurable options and control when we fire events or not.
|
Good idea, I added the relevant unit tests 👍. Skipped the ones that included sandbox events since they're not relevant for stories, but feel free to correct me if I'm wrong. /cc @zhouyx |
| if (observables) { | ||
| observables.fire(event); | ||
| } | ||
|
|
There was a problem hiding this comment.
sandboxEvent is added if an analytics event is added by an AMP component dynamically, in the format of sandbox-xxx.
In those case, the event won't be recognized as a story event because the name doesn't start with story-. Would that be an issue?
Code wise, it's fine extending the CustomEventTracker class, as no event would ever fall into the this.sandboxBuffer_ here.
There was a problem hiding this comment.
To make sure I understand: sandboxEvent is emitted by an arbitrary AMP component?
If that's the case then it should still go to the CustomEventTracker, right? So not intercepting sandbox-* events here seems like the right behavior.
Let me know if I'm missing something.
There was a problem hiding this comment.
That's right. All sandbox events will be handled by the CustomEventTracker.
It won't be an issue for now, but it could be an issue if a component trigger a sandbox story event in the future. sandbox-story-xxx. It would be great to keep that in mind by adding a comment.
Enriqe
force-pushed
the
story-event-tracker
branch
from
c653e6e
to
22d3d99
Compare
Enriqe
force-pushed
the
story-event-tracker
branch
from
93e7cf2
to
e22f15f
Compare
|
@zhouyx this is ready for another review 👍 |
| if (observables) { | ||
| observables.fire(event); | ||
| } | ||
|
|
There was a problem hiding this comment.
That's right. All sandbox events will be handled by the CustomEventTracker.
It won't be an issue for now, but it could be an issue if a component trigger a sandbox story event in the future. sandbox-story-xxx. It would be great to keep that in mind by adding a comment.
|
Thanks all! |
Partial rollforward of #23030
Doesn't include:
repeat: falseoption. This will come in a follow upDoes include:
AmpStoryEventTrackerwhich inherits fromCustomEventTracker. This will make it possible for us to build story-specific features in analytics (e.g.storySpec).