Simplify with-google-analytics example #43894

Josehower · 2022-12-09T16:18:12Z

Documentation / Examples

Make sure the linting passes by running pnpm build && pnpm lint
The "examples guidelines" are followed from our contributing doc

First of all thanks for this amazing project and all the help you provide with these examples.

It seems there is code duplication in this example. After some tests locally seem to _document.js is not necessary for gtag to work properly.

next.js/examples/with-google-analytics/pages/_app.js

Lines 30 to 34 in 9d97a1e

    
                   dangerouslySetInnerHTML={{ 
        
                     __html: ` 
        
                       window.dataLayer = window.dataLayer || []; 
        
                       function gtag(){dataLayer.push(arguments);} 
        
                       gtag('js', new Date());

next.js/examples/with-google-analytics/pages/_document.js

Lines 13 to 17 in 9d97a1e

    
                     dangerouslySetInnerHTML={{ 
        
                       __html: ` 
        
                       window.dataLayer = window.dataLayer || []; 
        
                       function gtag(){dataLayer.push(arguments);} 
        
                       gtag('js', new Date());

I am aware of #40645 and I would like to ask @dave-hay, @SukkaW and @ijjk to consider this is still necessary. If so why then not move all content of the scripts from _app to _document?

In any case, I am open to adding here some comments explaining what is the reason for this code duplication if necessary.

Changes that come from #43897

The event hashChangeComplete should be removed since /home and /home/#section is not new pageview, but just reference to the same page.

If we go from /home to /home/#section (with a button click or a link for example) this shouldn't trigger a new page visit on gtag.

For this reason, I think we should revert the changes from #36079. If there is a better argument of why this should stay I am also open to creating comments to clarify this on the example since I don't think should be the default behavior and not useful in most cases.

The id="gtag-init" was added with no context to the example from Add id to inline gtag script #29530

If there is a reason for this id in the script to existing I am open to adding a comment that clarifies this since in my experience is not necessary at all.

Edit: Batching with #43897 as recommended by #43897 (comment)

maxproske · 2022-12-09T16:23:18Z

FYI to maintainers, there's a PR open that's trying to add more to the _document.js. So it can be closed if this one is approved: #43838

dave-hay · 2022-12-09T19:42:18Z

@Josehower the reason for adding in _document.js was it resolved #40410, context below. If there have been updates to the framework since then that render this unnecessary then shouldn't be an issue to remove _document.js.

Describe the Bug

calling reportWebVitals when having google analytics setup using with-google-analytics results in Next.js-hydration event getting triggered BEFORE gtag is initialized.

This could be problematic in production if someone is just assuming gtag is always defined when reporting on web vitals.

Expected Behavior

When having google analytics setup using the with-google-analytics example, I would expect gtag to be defined for all events before triggering reportWebVitals.

Josehower · 2022-12-10T08:59:24Z

Batching with #43897 as recommended by #43897 (comment)

SukkaW · 2022-12-10T09:52:17Z

FYI to maintainers, there's a PR open that's trying to add more to the _document.js. So it can be closed if this one is approved: #43838

@maxproske @Josehower I just checked #43838 and I belive that PR is valid. Google Analytics does require a very lightweight inline script in the <head>. So _document is required for Google Analytics setup.

karlhorky · 2022-12-10T10:40:10Z

Google Analytics does require a very lightweight inline script in the <head>. So _document is required for Google Analytics setup.

Ok so if _document is required, maybe the suggestion in the description from @Josehower makes sense, to avoid the duplication mentioned in the description:

If [it's necessary to have _document] why then not move all content of the scripts from _app to _document?

If this is acceptable, the changes from #43838 could be also merged into this PR (or #43838 merged first, and then this PR rebased on that)

SukkaW · 2022-12-10T11:02:30Z

Ok so if _document is required, maybe the suggestion in the description from Josehower makes sense, to avoid the duplication mentioned in the description:

If [it's necessary to have _document] why then not move all content of the scripts from _app to _document?

Agree.

@Josehower Would you mind changing your PR to only remove gtag-init inside _app instead? So after #43838 and this PR merge, Google Analytics should work properly.

karlhorky · 2022-12-10T11:05:08Z

only remove gtag-init inside _app instead? So after #43838 and this PR merge, Google Analytics should work properly.

There would still be another change required - removing the duplicate code that is mentioned in the description of this PR:

next.js/examples/with-google-analytics/pages/_app.js

Lines 30 to 34 in 9d97a1e

    
                   dangerouslySetInnerHTML={{ 
        
                     __html: ` 
        
                       window.dataLayer = window.dataLayer || []; 
        
                       function gtag(){dataLayer.push(arguments);} 
        
                       gtag('js', new Date());

next.js/examples/with-google-analytics/pages/_document.js

Lines 13 to 17 in 9d97a1e

    
                     dangerouslySetInnerHTML={{ 
        
                       __html: ` 
        
                       window.dataLayer = window.dataLayer || []; 
        
                       function gtag(){dataLayer.push(arguments);} 
        
                       gtag('js', new Date());

karlhorky · 2022-12-10T11:07:58Z

Actually, I think the proposal by @Josehower was that all Google Analytics content be moved from _app to _document, so the parts below as well (why have these in _app when they can be in _document?).

Then the _app file can be removed completely.

next.js/examples/with-google-analytics/pages/_app.js

Lines 7 to 18 in 9d97a1e

    
           const router = useRouter() 
        
           useEffect(() => { 
        
             const handleRouteChange = (url) => { 
        
               gtag.pageview(url) 
        
             } 
        
             router.events.on('routeChangeComplete', handleRouteChange) 
        
             router.events.on('hashChangeComplete', handleRouteChange) 
        
             return () => { 
        
               router.events.off('routeChangeComplete', handleRouteChange) 
        
               router.events.off('hashChangeComplete', handleRouteChange) 
        
             } 
        
           }, [router.events])

next.js/examples/with-google-analytics/pages/_app.js

Lines 22 to 41 in 9d97a1e

    
                 {/* Global Site Tag (gtag.js) - Google Analytics */} 
        
                 <Script 
        
                   strategy="afterInteractive" 
        
                   src={`https://www.googletagmanager.com/gtag/js?id=${gtag.GA_TRACKING_ID}`} 
        
                 /> 
        
                 <Script 
        
                   id="gtag-init" 
        
                   strategy="afterInteractive" 
        
                   dangerouslySetInnerHTML={{ 
        
                     __html: ` 
        
                       window.dataLayer = window.dataLayer || []; 
        
                       function gtag(){dataLayer.push(arguments);} 
        
                       gtag('js', new Date()); 
        
                       gtag('config', '${gtag.GA_TRACKING_ID}', { 
        
                         page_path: window.location.pathname, 
        
                       }); 
        
                     `, 
        
                   }} 
        
                 />

karlhorky · 2022-12-10T11:12:46Z

only remove gtag-init inside _app instead?

Also, one more thing, there is another change in this PR aside from the gtag-init (because @maxproske suggested that the changes be batched - which probably created more confusion).

So this change to hashChangeComplete should also be considered (also described in the PR description), this has not been discussed yet:

Josehower · 2022-12-12T15:45:36Z

I manage to solve the issue with WebVitals reported on #40410 without the need of _document.js by adding the <script> on the <Head>

reproduction example of the fix: https://stackblitz.com/edit/nextjs-jyzaxg?file=pages/_app.js

This fix is also compliant with the suggestion done by @SukkaW on #43838 (review)

to summarize the changes proposed on this PR.

Remove code duplication on _app.js and _document.js by removing _document.js
Removing event hashChangeComplete since /home and /home/#section is not a new pageview but a reference to the same page and shouldn't be reported as different pages to google.
Removing id="gtag-init" that was added from Add id to inline gtag script #29530 now that we are not using a inline script but a normal script the id seems to be unnecessary

EDIT: adding more context to change number 3

SukkaW · 2022-12-16T13:55:51Z

examples/with-google-analytics/pages/_app.js

+            // Disable any default-enabled advertising features (turn them on later when we get consent)
+            gtag('set', 'allow_google_signals', false);


IMHO we shouldn't add this in the PR:

It is not required to set up Google Analytics

Anyone who follows Google Analytics' instruction can add this easily

This is not part of the PR anymore. ill update the description, sorry for that

https://github.com/vercel/next.js/pull/43894/files

maybe you are looking to an old version of the PR

This is the commit that removes this code 4 days ago

581c0af

ijjk

Thanks!

@dave-hay

## Documentation / Examples - [x] Make sure the linting passes by running `pnpm build && pnpm lint` - [x] The "examples guidelines" are followed from [our contributing doc](https://github.com/vercel/next.js/blob/canary/contributing/examples/adding-examples.md) First of all thanks for this amazing project and all the help you provide with these examples. It seems there is code duplication in this example. After some tests locally seem to _document.js is not necessary for `gtag` to work properly. https://github.com/vercel/next.js/blob/9d97a1e34a8a6e09eb127292c730d1a8df63ebb6/examples/with-google-analytics/pages/_app.js#L30-L34 https://github.com/vercel/next.js/blob/9d97a1e34a8a6e09eb127292c730d1a8df63ebb6/examples/with-google-analytics/pages/_document.js#L13-L17 I am aware of vercel#40645 and I would like to ask @dave-hay, @SukkaW and @ijjk to consider this is still necessary. If so why then not move all content of the scripts from _app to _document? In any case, I am open to adding here some comments explaining what is the reason for this code duplication if necessary. <hr/> Changes that come from vercel#43897 1. The `event` hashChangeComplete should be removed since `/home` and `/home/#section` is not new pageview, but just reference to the same page. If we go from /home to /home/#section (with a button click or a link for example) this shouldn't trigger a new page visit on `gtag`. For this reason, I think we should revert the changes from vercel#36079. If there is a better argument of why this should stay I am also open to creating comments to clarify this on the example since I don't think should be the default behavior and not useful in most cases. 2. The `id="gtag-init"` was added with no context to the example from vercel#29530 If there is a reason for this id in the script to existing I am open to adding a comment that clarifies this since in my experience is not necessary at all. Edit: Batching with vercel#43897 as recommended by vercel#43897 (comment) --------- Co-authored-by: JJ Kasper <jj@jjsweb.site>

Remove unnecesary file

4854604

Josehower requested review from timneutkens, ijjk, shuding, leerob, steven-tey and balazsorban44 as code owners December 9, 2022 16:18

ijjk added the examples Issue/PR related to examples label Dec 9, 2022

Josehower added 5 commits December 9, 2022 17:39

Remove unnecesary code for the example

bc4f914

Move back event since is necessary

6dbcf1a

Remove hashChangeComplete event

0e504e3

Reorder imports

a324e70

Move back code style

9e1769f

maxproske mentioned this pull request Dec 9, 2022

Remove unnecessary code for the example #43897

Closed

2 tasks

Merge branch 'remove-unnecesary-id' into remove-unnecesary-document-file

7aa3e46

Josehower closed this Dec 10, 2022

Josehower reopened this Dec 10, 2022

Josehower changed the title ~~Remove unnecessary _document.js~~ Remove unnecessary code in _app.js and _document.js Dec 10, 2022

Josehower added 2 commits December 12, 2022 16:33

Solve webVitals issue with no _document.js

d7138c0

Move the comment to the right place

0268324

Josehower added 3 commits December 12, 2022 16:55

Remove configuration that was not in example

581c0af

Move back code styles

cab512e

Run prettier

4ce3056

Josehower mentioned this pull request Dec 16, 2022

Move Google Analytics script to the <Head/> #43838

Merged

11 tasks

Josehower added 3 commits December 16, 2022 14:35

Merge branch 'canary' into remove-unnecesary-document-file

29b9945

Remove semicolon

eef2106

Fix indentation

347db20

SukkaW suggested changes Dec 16, 2022

View reviewed changes

ijjk approved these changes Feb 7, 2023

View reviewed changes

Merge branch 'canary' into remove-unnecesary-document-file

a19eea2

ijjk requested a review from jankaifer as a code owner February 7, 2023 22:40

ijjk changed the title ~~Remove unnecessary code in _app.js and _document.js~~ Simplify with-google-analytics example Feb 7, 2023

ijjk merged commit 268dd6e into vercel:canary Feb 7, 2023

github-actions bot locked as resolved and limited conversation to collaborators Mar 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify with-google-analytics example #43894

Simplify with-google-analytics example #43894

Josehower commented Dec 9, 2022 •

edited

maxproske commented Dec 9, 2022

dave-hay commented Dec 9, 2022

Describe the Bug

Expected Behavior

Josehower commented Dec 10, 2022

SukkaW commented Dec 10, 2022 •

edited

karlhorky commented Dec 10, 2022 •

edited

SukkaW commented Dec 10, 2022

karlhorky commented Dec 10, 2022 •

edited

karlhorky commented Dec 10, 2022 •

edited

karlhorky commented Dec 10, 2022 •

edited

Josehower commented Dec 12, 2022 •

edited

SukkaW Dec 16, 2022

Josehower Dec 16, 2022

Josehower Dec 16, 2022 •

edited

Josehower Dec 16, 2022

ijjk left a comment

	dangerouslySetInnerHTML={{
	__html: `
	window.dataLayer = window.dataLayer \|\| [];
	function gtag(){dataLayer.push(arguments);}
	gtag('js', new Date());

		// Disable any default-enabled advertising features (turn them on later when we get consent)
		gtag('set', 'allow_google_signals', false);

Simplify with-google-analytics example #43894

Simplify with-google-analytics example #43894

Conversation

Josehower commented Dec 9, 2022 • edited

Documentation / Examples

maxproske commented Dec 9, 2022

dave-hay commented Dec 9, 2022

Describe the Bug

Expected Behavior

Josehower commented Dec 10, 2022

SukkaW commented Dec 10, 2022 • edited

karlhorky commented Dec 10, 2022 • edited

SukkaW commented Dec 10, 2022

karlhorky commented Dec 10, 2022 • edited

karlhorky commented Dec 10, 2022 • edited

karlhorky commented Dec 10, 2022 • edited

Josehower commented Dec 12, 2022 • edited

SukkaW Dec 16, 2022

Choose a reason for hiding this comment

Josehower Dec 16, 2022

Choose a reason for hiding this comment

Josehower Dec 16, 2022 • edited

Choose a reason for hiding this comment

Josehower Dec 16, 2022

Choose a reason for hiding this comment

ijjk left a comment

Choose a reason for hiding this comment

Josehower commented Dec 9, 2022 •

edited

SukkaW commented Dec 10, 2022 •

edited

karlhorky commented Dec 10, 2022 •

edited

karlhorky commented Dec 10, 2022 •

edited

karlhorky commented Dec 10, 2022 •

edited

karlhorky commented Dec 10, 2022 •

edited

Josehower commented Dec 12, 2022 •

edited

Josehower Dec 16, 2022 •

edited