Skip to content

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