-
Notifications
You must be signed in to change notification settings - Fork 40
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
feat: add auto-tracking plugin #570
Conversation
packages/plugin-ga-events-forwarder-browser/test/ga-events-forwarder.test.ts
Show resolved
Hide resolved
32d0ef8
to
5987885
Compare
5987885
to
d3cf27c
Compare
interface Options { | ||
cssSelectorAllowlist?: string[]; | ||
tagAllowlist?: string[]; | ||
ingestionMetadata?: IngestionMetadata; // Should avoid overriding this if unplanned to do so, this is not available in the charts. |
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.
@liuyang1520 instead of including ingestionMetadata
on the plugin Options
, can it instead be retrieved in setup()
via the client config?
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.
Thanks @justin-fiedler ! Good call, I can remove this override and use the existing client config one.
<br /> | ||
</p> | ||
|
||
# @amplitude/plugin-auto-tracking-browser |
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.
can we get this name approved by growth and dx teams (or other stakeholders)?
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.
Good call! This plugin is for internal use at present, will definitely need more discussion/approval about the naming and interface if we want to make it ready to public users.
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.
are we publishing to npm? it'll still be public if so. and we can't rename packages in npm
cssSelectorAllowlist: ['.amp-auto-tracking', '[amp-auto-tracking]'], | ||
tagAllowlist: ['button', 'a'], |
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.
do these need to be separate or can we unify?
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 thought about this. The function is a little bit overlapping but not all are easy to configure. For tagAllowlist
, if it is not provided, we have a list of default tags that we want to track, while cssSelectorAllowlist
is for more advanced usage, if doesn't have a default list. If we just rely on cssSelectorAllowlist
with a default list like ['button', 'a']
, then problem is, when we want to add a allowed class, we have to do ['button.amp-auto-tracking', 'a.amp-auto-tracking']
, need to explicitly copy all tags here (this is what I think not easy to configure). Let me know if you have any good thoughts on this, this interface is not ready to public users at the moment though.
Summary
Extract the auto-tracking logic from #529 to a new package.
Instantiate the plugin
The plugin accepts 1 optional parameter, which is an
Object
to configure the allowed tracking options.Examples:
cssSelectorAllowlist
will only allow tracking elements like:<button amp-auto-tracking>Click</button>
<a class="amp-auto-tracking">Link</a>
tagAllowlist
will only allowbutton
anda
tags to be tracked.Note
ingestionMetadata
is for internal use only, you don't need to provide it.Options
cssSelectorAllowlist
string[]
undefined
tagAllowlist
string[]
['a', 'button', 'input', 'select', 'textarea', 'label']
Checklist