Skip to content

Include add_payment_info and page_view events #2276

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

Merged
merged 2 commits into from
Oct 25, 2019
Merged

Include add_payment_info and page_view events #2276

merged 2 commits into from
Oct 25, 2019

Conversation

cdoe
Copy link
Contributor

@cdoe cdoe commented Oct 17, 2019

Typescript currently throws a fit when trying to log either the add_payment_info or page_view events with firebase.analytics.logEvent(). The error goes a little something like...

Argument of type '"add_payment_info"' is not assignable to parameter of type 'never'.

These two are the only default events without parameters and also don't have function overloads defined—meaning the following line assigns them to never.

export type CustomEventName<T> = T extends EventNameString ? never : T;

I think adding these two definitions patches things right up.

Copy link
Contributor

@hsubox76 hsubox76 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, you're right that we need to add those signatures. One small thing I'd like to ask for is to fill out the eventParams in each with event-appropriate suggested (optional) params, to provide useful autocomplete hints. (see code comments below)

I think that's it and then I will be happy to merge!

*/
logEvent(
eventName: 'page_view',
eventParams?: {},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For page_view I think it should be:

eventParams: {
        page_title?: string;
        page_location?: string;
        page_path?: string;
        [key: string]: any;
      }

Source: https://developers.google.com/analytics/devguides/collection/gtagjs/pages

@cdoe
Copy link
Contributor Author

cdoe commented Oct 23, 2019

Thanks for pointing that out, @hsubox76. New commit corrects those two eventParams. And sorry for some of the line-break weirdness. I think some extra spaces were included in my first commit that are now removed with this new one.

@cdoe cdoe requested a review from hsubox76 October 23, 2019 18:21
Copy link
Contributor

@hsubox76 hsubox76 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for the contribution!

@hsubox76 hsubox76 merged commit bd3a03a into firebase:master Oct 25, 2019
@hsubox76 hsubox76 added this to the next milestone Oct 29, 2019
@cdoe cdoe deleted the patch-1 branch October 29, 2019 23:23
@firebase firebase locked and limited conversation to collaborators Nov 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants