-
Notifications
You must be signed in to change notification settings - Fork 27.2k
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
Fix sendGAEvent function #62065
Fix sendGAEvent function #62065
Conversation
…nstead of using rest parameter syntax
Allow CI Workflow Run
Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer |
Stats from current PRDefault Build (Increase detected
|
vercel/next.js canary | willashe/next.js send-ga-event-fix | Change | |
---|---|---|---|
buildDuration | 19.9s | 19.9s | N/A |
buildDurationCached | 8.5s | 7.1s | N/A |
nodeModulesSize | 196 MB | 196 MB | N/A |
nextStartRea..uration (ms) | 423ms | 430ms | N/A |
Client Bundles (main, webpack)
vercel/next.js canary | willashe/next.js send-ga-event-fix | Change | |
---|---|---|---|
1068-HASH.js gzip | 30.3 kB | 30.2 kB | N/A |
3f784ff6-HASH.js gzip | 53.5 kB | 53.5 kB | N/A |
4944-HASH.js gzip | 5.04 kB | 5.03 kB | N/A |
8423.HASH.js gzip | 181 B | 181 B | ✓ |
framework-HASH.js gzip | 45.2 kB | 45.2 kB | ✓ |
main-app-HASH.js gzip | 241 B | 242 B | N/A |
main-HASH.js gzip | 32.1 kB | 32.1 kB | N/A |
webpack-HASH.js gzip | 1.7 kB | 1.7 kB | ✓ |
Overall change | 47.1 kB | 47.1 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | willashe/next.js send-ga-event-fix | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
Overall change | 31 kB | 31 kB | ✓ |
Client Pages
vercel/next.js canary | willashe/next.js send-ga-event-fix | Change | |
---|---|---|---|
_app-HASH.js gzip | 196 B | 196 B | ✓ |
_error-HASH.js gzip | 184 B | 183 B | N/A |
amp-HASH.js gzip | 503 B | 504 B | N/A |
css-HASH.js gzip | 323 B | 324 B | N/A |
dynamic-HASH.js gzip | 2.5 kB | 2.51 kB | N/A |
edge-ssr-HASH.js gzip | 258 B | 259 B | N/A |
head-HASH.js gzip | 353 B | 351 B | N/A |
hooks-HASH.js gzip | 370 B | 370 B | ✓ |
image-HASH.js gzip | 4.21 kB | 4.2 kB | N/A |
index-HASH.js gzip | 259 B | 259 B | ✓ |
link-HASH.js gzip | 2.68 kB | 2.67 kB | N/A |
routerDirect..HASH.js gzip | 313 B | 314 B | N/A |
script-HASH.js gzip | 386 B | 385 B | N/A |
withRouter-HASH.js gzip | 309 B | 311 B | N/A |
1afbb74e6ecf..834.css gzip | 106 B | 106 B | ✓ |
Overall change | 931 B | 931 B | ✓ |
Client Build Manifests
vercel/next.js canary | willashe/next.js send-ga-event-fix | Change | |
---|---|---|---|
_buildManifest.js gzip | 485 B | 484 B | N/A |
Overall change | 0 B | 0 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | willashe/next.js send-ga-event-fix | Change | |
---|---|---|---|
index.html gzip | 530 B | 527 B | N/A |
link.html gzip | 542 B | 540 B | N/A |
withRouter.html gzip | 525 B | 522 B | N/A |
Overall change | 0 B | 0 B | ✓ |
Edge SSR bundle Size
vercel/next.js canary | willashe/next.js send-ga-event-fix | Change | |
---|---|---|---|
edge-ssr.js gzip | 94.5 kB | 94.4 kB | N/A |
page.js gzip | 151 kB | 151 kB | N/A |
Overall change | 0 B | 0 B | ✓ |
Middleware size
vercel/next.js canary | willashe/next.js send-ga-event-fix | Change | |
---|---|---|---|
middleware-b..fest.js gzip | 625 B | 628 B | N/A |
middleware-r..fest.js gzip | 151 B | 151 B | ✓ |
middleware.js gzip | 44.6 kB | 44.4 kB | N/A |
edge-runtime..pack.js gzip | 1.94 kB | 1.94 kB | ✓ |
Overall change | 2.1 kB | 2.1 kB | ✓ |
Next Runtimes
vercel/next.js canary | willashe/next.js send-ga-event-fix | Change | |
---|---|---|---|
app-page-exp...dev.js gzip | 166 kB | 166 kB | N/A |
app-page-exp..prod.js gzip | 95.9 kB | 95.5 kB | N/A |
app-page-tur..prod.js gzip | 97.6 kB | 97.3 kB | N/A |
app-page-tur..prod.js gzip | 92 kB | 91.7 kB | N/A |
app-page.run...dev.js gzip | 136 kB | 136 kB | N/A |
app-page.run..prod.js gzip | 90.6 kB | 90.3 kB | N/A |
app-route-ex...dev.js gzip | 22 kB | 22 kB | N/A |
app-route-ex..prod.js gzip | 14.9 kB | 14.9 kB | N/A |
app-route-tu..prod.js gzip | 14.9 kB | 14.9 kB | N/A |
app-route-tu..prod.js gzip | 14.6 kB | 14.6 kB | N/A |
app-route.ru...dev.js gzip | 21.7 kB | 21.7 kB | N/A |
app-route.ru..prod.js gzip | 14.6 kB | 14.6 kB | N/A |
pages-api-tu..prod.js gzip | 9.47 kB | 9.43 kB | N/A |
pages-api.ru...dev.js gzip | 9.74 kB | 9.7 kB | N/A |
pages-api.ru..prod.js gzip | 9.47 kB | 9.43 kB | N/A |
pages-turbo...prod.js gzip | 22.1 kB | 22 kB | N/A |
pages.runtim...dev.js gzip | 22.7 kB | 22.7 kB | N/A |
pages.runtim..prod.js gzip | 22.1 kB | 22 kB | N/A |
server.runti..prod.js gzip | 50.1 kB | 50 kB | N/A |
Overall change | 0 B | 0 B | ✓ |
build cache Overall increase ⚠️
vercel/next.js canary | willashe/next.js send-ga-event-fix | Change | |
---|---|---|---|
0.pack gzip | 1.55 MB | 1.55 MB | N/A |
index.pack gzip | 103 kB | 103 kB | |
Overall change | 103 kB | 103 kB |
Diff details
Diff for page.js
Diff too large to display
Diff for middleware.js
Diff too large to display
Diff for edge-ssr.js
Diff too large to display
Diff for 1068-HASH.js
Diff too large to display
Diff for main-HASH.js
Diff too large to display
Diff for app-page-exp..ntime.dev.js
failed to diff
Diff for app-page-exp..time.prod.js
Diff too large to display
Diff for app-page-tur..time.prod.js
Diff too large to display
Diff for app-page-tur..time.prod.js
Diff too large to display
Diff for app-page.runtime.dev.js
failed to diff
Diff for app-page.runtime.prod.js
Diff too large to display
Diff for app-route-ex..ntime.dev.js
Diff too large to display
Diff for app-route-ex..time.prod.js
Diff too large to display
Diff for app-route-tu..time.prod.js
Diff too large to display
Diff for app-route-tu..time.prod.js
Diff too large to display
Diff for app-route.runtime.dev.js
Diff too large to display
Diff for app-route.ru..time.prod.js
Diff too large to display
Diff for pages-api-tu..time.prod.js
Diff too large to display
Diff for pages-api.runtime.dev.js
Diff too large to display
Diff for pages-api.ru..time.prod.js
Diff too large to display
Diff for pages-turbo...time.prod.js
Diff too large to display
Diff for pages.runtime.dev.js
Diff too large to display
Diff for pages.runtime.prod.js
Diff too large to display
Diff for server.runtime.prod.js
Diff too large to display
Looks like TS isn't happy. Working on a fix. |
Wouldn't it be enough to change the |
Can confirm that this solution solved the issue for me. This approach of using At least that's what seemed to work for me when testing. And could be a good thing, because then any |
if (currDataLayerName === undefined) { | ||
console.warn(`@next/third-parties: GA has not been initialized`) | ||
return | ||
} | ||
|
||
if (window[currDataLayerName]) { | ||
window[currDataLayerName].push(arguments) | ||
window[currDataLayerName].push(args) |
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.
We might want to leave this one as arguments
. It seems like this does not work with Arrays, but works with arguments object. Additionally, when using window.gtag(...)
an Arguments()
is pushed instead of Array()
.
Might be related, see: https://stackoverflow.com/questions/60400130/google-analytics-datalayer-push-not-working-with-array-of-variables#60410151
This reverts commit e71046b.
The recently merged PR - #62065, doesn't seem to fix the issue. As per the comment this seems to be related to `dataLayer` that does not like working with Arrays, but works with [arguments object](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/arguments). Also, when using window.gtag(...) an Arguments() is pushed instead of Array(). Fixes #61703
This doesn't seem to work for me, maybe I've got the usage wrong. I'm using next/third-parties v14.1.1 and I'm calling
I'm seeing weird stuff in the DataLayer and I do NOT see /collect endpoint hits. I've also tried using the syntax from the docs ( |
Hey @daynejones, just to double check:
The usage seems to be ok |
Thanks for the response @asobirov Maybe that's the problem. I've got GTM and GA running (it's a third party's requirement). I see a 200 for gtm itself and I see |
I think the issue might just be the package version. Tried out the
I'm guessing that you should be good using the pre-release version - |
Got it. Will try that and report back. |
Yep, works now. Thanks for the help! |
Using 14.1.2 and it's still not working for me. What version are you in exactly? |
You need to use a canary version for now, for example I’m using 14.1.2-canary.7 |
It's not the way to fix sending event to GA. |
For those following, this is still not fixed nor the docs corrected in main (unsure why it wasn't backported since it's a regression/bug?). You must use |
Is this working for anyone in v14.2.0-canary.34? |
@craigRSA yeah, works for me! |
@dariuscosden
|
@craigRSA yes exactly. However I'm sending it using |
it works when running from my local but once its build in prod on Vercel it stops working. Other events work fine, just not the custom ones. It's a client component. Annoying.. |
Same for me! it run in local but non in prod, but not on Vercel. |
@m1dok we used GTM to forward the requests to GA via a trigger and event parameters to send through all the fields we wanted |
@craigRSA thanks, it work! :) |
Previous implementation of
sendGAEvent
was pushing arguments todataLayer
via rest parameter syntax, resulting in an actual array, which GA doesn't seem to process correctly (resulting in the event not showing up in GA reports). This PR refactors it to a vanilla JS function passing data via thearguments
array-like object, which is able to be processed correctly and results in the event showing up as expected.fixes 61703