Skip to content
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 typings for TypeScript interop #11

Merged
merged 1 commit into from
Apr 30, 2019
Merged

Conversation

rand0me
Copy link
Contributor

@rand0me rand0me commented Apr 4, 2019

Hi, Luke
Epic thanks for this wonderful alternative to default GA client!

I use this package with TypeScript and while it hasn't typings I forced to copy-paste my types.d.ts from project to project. It would be great if you merge this PR to make this awesome library support TypeScript.

Thanks!

Copy link
Owner

@lukeed lukeed left a comment

Choose a reason for hiding this comment

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

This is awesome! 🙌 Thank you so much & for the kind words 😃

Going to leave a few comments. While I'm sure these work, I'd like to mold them into a setup I'm more familiar with/used to seeing.... largely because I don't actually use TS but I can speak to/debug on behalf of what I've worked with before.

PS: Sorry for the delay on reviewing. I've been chasing down other rabbit holes

ganalytics.d.ts Outdated
@@ -0,0 +1,53 @@
interface IGAnalyticsOptions {
Copy link
Owner

Choose a reason for hiding this comment

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

Let's export this

ganalytics.d.ts Outdated
ds?: string;
}

interface IGAnalytics {
Copy link
Owner

Choose a reason for hiding this comment

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

Let's also export this

ganalytics.d.ts Outdated
* @param options Options
* @param toWait When truthy, a pageview event will not be sent immediately upon initialization.
*/
function GAnalyticsFactory(trackerID: string, options?: IGAnalyticsOptions, toWait?: boolean): IGAnalytics;
Copy link
Owner

Choose a reason for hiding this comment

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

I think this can be options?: Partial<IGAnalyticsOptions> ?

ganalytics.d.ts Outdated
}

interface IGAnalytics {
send: (kind: "pageview" | "event", params?: any) => void;
Copy link
Owner

Choose a reason for hiding this comment

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

There's actually a lot more kinds that can be sent. Here's the list: https://github.com/lukeed/ganalytics#type

So let's add a new Enum here:

send: (kind: EventType, params?: object) => void

Also, params can only be an object.

* Indicates the data source type of the hit; eg web or app.
* See [Data Source](https://developers.google.com/analytics/devguides/collection/protocol/v1/parameters#ds).
*/
ds?: string;
Copy link
Owner

Choose a reason for hiding this comment

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

With the Partial keyword, we don't have to add ? to every key

@lukeed
Copy link
Owner

lukeed commented Apr 8, 2019

The additional exports are because I know that someone will undoubtedly ask for them, so we might as well jump the gun here 😆

As for general setup, I don't think we necessarily need the namespace? Might be mistaken here. But GAnalytics will work with/without new keyword, so technically, I don't think a factory need be returned at all.

export type EventType = 'send' | 'pageview' | ...;

export interface Options {
  // ...
}

export interface GAnalytics {
  send: (type: EventType, params?: object) => void;
}

declare const ganalytics: (trackerID: string, options?: Partial<Options>, toWait?: boolean): GAnalytics;

export default ganalytics;

@rand0me
Copy link
Contributor Author

rand0me commented Apr 9, 2019

Hi, @lukeed ! 👋
I have just updated typings according to your review and also did overloads for send() method, so it could be easily used with different event types utilizing params interfaces.

Also, I modified commit message according to yours

@rand0me
Copy link
Contributor Author

rand0me commented Apr 30, 2019

Hi @lukeed! 👋

How about merging this PR?
I will be very thankful if you accept it! :)

@lukeed
Copy link
Owner

lukeed commented Apr 30, 2019

Hi! So sorry 🙈 thank you for the ping!

This is super awesome & very very much appreciated 😄 I was using it today and found some errors and incomplete sections, but I will merge this as is ATM. I'll apply the fixes soon after – likely today. I have a bunch of them already locally.

Thank you thank you 🙌

@lukeed lukeed merged commit 1666f00 into lukeed:master Apr 30, 2019
@rand0me
Copy link
Contributor Author

rand0me commented May 1, 2019

Thanks a bunch!
Very kind from you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants