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 example with fanthom analytics #403

Merged

Conversation

xstevenyung
Copy link
Contributor

simple fanthom analytics example via CDN, would love feedback if there is stuff missing on this. i'm currently implementing this on my project so any improvement would be greatly appreciated

Test Plan

Checklist

  • Tests updated
  • Docs updated

Screenshots

Copy link
Member

@kentcdodds kentcdodds 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 this! There are a few more things that should probably be handled in your example:

  1. Update the CSP to support the fathom script: https://github.com/kentcdodds/kentcdodds.com/blob/5dc7a5e6f0561023be3b348c5bf5fd88f3391eb3/server/index.ts#L247
  2. Handle client-side pageviews: https://github.com/kentcdodds/kentcdodds.com/blob/5dc7a5e6f0561023be3b348c5bf5fd88f3391eb3/app/root.tsx#L384-L419
  3. Handle pre-fathom-loaded function calls with a fathom queue: https://github.com/kentcdodds/kentcdodds.com/blob/5dc7a5e6f0561023be3b348c5bf5fd88f3391eb3/app/root.tsx#L525-L535

Hope that helps! Thanks for making the example!

@xstevenyung
Copy link
Contributor Author

made some changes to implement everything but made some modification and would love your feedback again to suit the epic stack to the maximum:

  • move all the code (script tag component + code related to fathom) into a utils/fathom.tsx
  • i saw that your <script /> for fathom on your repo uses onLoad, from all my testing the onLoad never triggered. i made some research on this but couldn't confirm if this is a bug or if i screw up on my end. for the time being, i moved everything into a load event listener until i can figure it out
  • also added all the other methods available in fathom (trackGoal, enableTrackingForMe, etc.)
  • change the queue system from command (which was hard to type) to queue of function as we are on the client-side only. this ease the typescript and execution of the queue on load.

@kentcdodds
Copy link
Member

Looking great. I love the improvements! I left a couple comments.

@xstevenyung
Copy link
Contributor Author

made it work with a proxy and also changed the useFathom to return a getter function to be able to get the fathom instance or the queue proxy at the moment of using it.

not sure how I feel about it when I feel like this is fine. it's a lot of complexity added for little benefits but I will let you be the judge.

if there is any other way you have in mind, would take it.

I made a separate PR to easily compare the two options
https://github.com/xstevenyung/epic-stack-with-fathom-analytics/pull/1/files

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Love it! Once you merge that proxy solution then I'll merge this 👍👍 Thanks a lot! I think we'll probably bring this into the core eventually.

@xstevenyung
Copy link
Contributor Author

updated 👌

@kentcdodds kentcdodds merged commit f912953 into epicweb-dev:main Sep 6, 2023
5 checks passed
@kentcdodds
Copy link
Member

Thanks!

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