-
Notifications
You must be signed in to change notification settings - Fork 218
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
Add the Event Specifications plugin #1300
Conversation
3a9e2e2
to
618442e
Compare
BundleMonFiles added (6)
Total files change +99.17KB 0% Final result: ✅ View report in BundleMon website ➡️ |
618442e
to
639abcc
Compare
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 like this, seems like a nice solution!
Just one question, did you consider having a typed interface for the EventSpecificationsPluginOptions
constructor parameter? Currently users can pass Record<string, string>
for the specific plugin events, which opens room for typos (e.g., play_evnt
) and doesn't show the users what are the available options. It is more flexible for us this way, so it's a trade-off, just wanted to hear your thoughts on that.
activateBrowserPlugin: (tracker) => { | ||
trackerId = tracker.id; | ||
_trackers[trackerId] = tracker; | ||
return; |
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.
Nit: extra return
Good call! This is also mentioned in the README and will be added in the documentation page as well.
Does it make sense? |
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.
That makes sense! If it's going to be primarily used by copying from Console, then it should be fine this way.
Except for the one potential problem that I commented, this LGTM!
if (!matchedEvent) { | ||
return; | ||
} | ||
const matchingPluginEventMap = Object.values(options).find( |
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.
Why do we consider all integrations here? For instance if the user configured the plugin like this:
EventSpecificationPlugin({
OtherPlugin: { play_event: "wrong" },
SnowplowMediaPlugin: { play_event: "correct" },
})
And the current event was matched against the media plugin, wouldn't this code result in the wrong
ES ID being taken?
This can't happen now since we only have one integration, but if there is such a problem, it would be good to solve it before we add more integrations.
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.
Yeap, fixed:
const matchingPluginEventMap = options[integrationKey as AvailablePlugins];
const matchingEventSpecificationId = matchingPluginEventMap && matchingPluginEventMap[matchedEvent];
if (!matchingEventSpecificationId) {
return;
}
Plus both integrations and options can only have AvailablePlugins
as keys
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!
639abcc
to
b9c45bd
Compare
b9c45bd
to
5d531a6
Compare
No description provided.