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

Better way to add the { analytics } prop type to NextPage? #52

Open
omarryhan opened this issue May 24, 2020 · 3 comments
Open

Better way to add the { analytics } prop type to NextPage? #52

omarryhan opened this issue May 24, 2020 · 3 comments
Labels
help wanted Extra attention is needed

Comments

@omarryhan
Copy link

Thanks for the awesome package!

Minor question. Is there a way to make TS infer the extra { analytics } prop without importing my custom _App props in every page?

Also, is there a specific reason why the Readme.md is in JS and not TS? I can help add TS code instructions if you're open to it.

@omarryhan
Copy link
Author

I'm a bit confused, why use the analytics prop and the attached helpers when you can just import ReactGA and use it directly? Is there something I'm missing?

If there's no difference, then I think that importing ReactGA should be the recommended way. What do you think?

@omarryhan
Copy link
Author

Two more things 😅,

  1. In prod.ts and dev.ts, you're using:
if (typeof window !== 'undefined') {
    ... doSth()
}

inconsistently. At first, I thought that was not to make the app fail on build time (when there is no window object) or when SSRing. But then, I saw that it worked fine with the real ReactGA object.

  1. I think it's better to directly expose React GA's interfaces. It will make your life easier as a maintainer because you won't have to keep updating types whenever ReactGA do any changes to their interface.

Same goes with WithAnalyticsConfig It's better to use ReactGA's own config interface.

Let me know what you think. If you agree, I can work on a PR to address these issues.

@resir014
Copy link
Collaborator

@omarryhan Hey, sorry for the delayed response.

I initially set it that way since I had plans to include the ability to hook in other analytics solutions, but since we're focusing on the GA experience right now we might accept your changes.

If you're still up for it, feel free to send a PR!

@resir014 resir014 added the help wanted Extra attention is needed label Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants