Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
refactor(plugin-gtag): update gtag plugin to modern SPA recommendations #8143
refactor(plugin-gtag): update gtag plugin to modern SPA recommendations #8143
Changes from 9 commits
2382178
9ea3476
e3b9137
cda7b8a
961966e
b4780e2
1631f43
1bd3edf
bd0683e
88e9407
6d49f61
3cb8773
8ce5d72
be36601
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we still need this TS if we have gtag.js typedefs? Why?
This dependency is supposed to provide all the types we need
Have you tried using my recos like the /// directive?
Looks similar to what someone suggest here:
https://andrew-simpson-ross.medium.com/strongly-typed-google-analytics-v4-with-next-js-aad6c6a5e383
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure but probably not. I don't understand the intricacies of typedefs and the original purpose of this file enough to say with confidence, but yeah, its probably superfluous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't understand why we need this abstraction in the first place.
The previous conversation is not resolved: if you can't explain clearly why we need this, we won't add it 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows developers to send custom events to GA without having to go through the
gtag.configure
call all over again.We have added some functionality to our DS sites that does not result in a navigation event (basically it displays or hides some content on the page based off a click event). We need to be able to track those clicks. To do this, we need to send custom events to GA.
With this hook the code needed to send the custom events is much cleaner and easier, we don't have to copy/paste the
configure
boilerplate in individual React components, its type-safe and does not requirets-ignore
statements for thewindow.gtag()
call, nor do we need to create an internal service to reduce the boilerplate. Or, more specifically, I decided instead of writing that service for only our internal use I figured I'd give back to the community.Also, its best to avoid re-calling
configure
because every time theconfigure
call is made a new pageview event is generated unless its disabled as documented here. Without disabling the default behavior pageview events will be doubled in count. While easy to disable, its also very easy to forget to disable it (ask me how I know that 😅).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what you mean by "configure", I only see this in the doc:
gtag("config",...)
This is already handle in the plugin:
There's no reason your code should call this "config" action more than once IMHO, or you'd have to explain to me what you want to achieve exactly by calling configure multiple times. It's already done for you, and you can forget about it.
Just write this in your code:
No React hook is needed to do that
And if you wire gtag.js types properly, it should also be typesafe.
Now if you really want to have a React hook, you can create it directly in your own codebase. This might be useful for example if you have a TS union type of "allowed custom events" and you want to provide event name autocompletion or typo prevention on custom event names. But that's outside the scope of this plugin and should be handled on a case-by-case basis