refactor(plugin-gtag): update gtag plugin to modern SPA recommendations #8143

lanegoolsby · 2022-09-28T20:48:34Z

Pre-flight checklist

I have read the Contributing Guidelines on pull requests.
If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

This exposes a React hook that allows for programmatically sending custom events to Google Analytics.

Test Plan

Temporarily added a button to the home page with a onClick that called the hook.

//snip
import { useGoogleAnalytics, type Event } from '@docusaurus/plugin-google-gtag';
//snip

function HeroBanner() {
  const { sendEvent } = useGoogleAnalytics();

  function onClick() {
    const event: Event = {
      action: 'some action',
      event_category: 'come category',
      event_label: 'some label',
      value:'some value'
    }
    sendEvent(event)
  }

  return (
    <div className={styles.hero} data-theme="dark">
      <div className={styles.heroInner}>
        <button type='button' onClick={onClick}><Translate>Click me</Translate></button>

Test links

Deploy preview: https://deploy-preview-_____--docusaurus-2.netlify.app/

Related issues/PRs

netlify · 2022-09-28T20:52:50Z

✅ [V2]

Name	Link
🔨 Latest commit	`be36601`
🔍 Latest deploy log	https://app.netlify.com/sites/docusaurus-2/deploys/6352814938100b0008b0522c
😎 Deploy Preview	https://deploy-preview-8143--docusaurus-2.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

github-actions · 2022-09-28T20:53:37Z

⚡️ Lighthouse report for the deploy preview of this PR

URL	Performance	Accessibility	Best Practices	SEO	PWA	Report
/	🟠 67	🟢 98	🟢 100	🟢 100	🟠 80	Report
/docs/installation	🟠 74	🟢 100	🟢 100	🟢 100	🟢 90	Report

packages/docusaurus-plugin-google-gtag/src/gtag.ts

slorber · 2022-09-29T14:44:43Z

packages/docusaurus-plugin-google-gtag/src/utils/useGoogleAnalytics.ts

+export function useGoogleAnalytics(): {
+  sendEvent: (event: Event) => void;
+} {
+  const sendEvent = useCallback((event: Event) => {
+    window.gtag('event', event.action, event);
+  }, []);
+
+  return {sendEvent};
+}


I'm not sure to understand the value of this abstraction 🤷‍♂️

What prevents someone to just do this directly in their code? (which would give them even more flexibility)

function onClick() { const event = { action: 'some action', event_category: 'come category', event_label: 'some label', value:'some value' } window.gtag('event', event.action, event); }

They can, I'm just an old school OOO programmer and will always provide strong types. :)

I can agree with that, but the types should rather be provided by ambient definitions from official gtag TS bindings preferably.

If they don't provide those, I'd rather create our own types only, and not add an extra runtime api/hook

For example:

interface Window { gtag: ... }

Isn't this good enough to provide strong types for our users? Do we really need the extra client api/hook?

I can agree with that, but the types should rather be provided by ambient definitions from official gtag TS bindings preferably.

If they don't provide those, I'd rather create our own types only, and not add an extra runtime api/hook

There are gtag types published to DefinitelyTyped. Are you suggesting I get rid of the Event interface and replace it with something exported from there?

For example:

interface Window { gtag: ... }

Isn't this good enough to provide strong types for our users? Do we really need the extra client api/hook?

I couldn't get that approach to work with my code. Its possible I was doing something wrong, but I always got a syntax error that gtag does not exist on the Window object.

In fact, the reason that I don't reference the Event interface in src/types.d.ts is because if I try to use an import statement in that file I get the same syntax error.

The onClick approach you suggested above does indeed work, but it requires using a ts-ignore statement because gtag is not global. Our internal code analyzers get very angry when we do stuff like that.

PS - there is an unofficial React gtag package available. Its basically a port of the popular ReactGA plugin (that only works with old GA3 sites).

I thought about going that route instead but it seemed like a much bigger lift that this approach.

If we ever import a third-party package, it should be either the official one of the DT one.

This one looks appropriate to me:

https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/gtag.js

https://www.npmjs.com/package/@types/gtag.js

As far as I know adding a @types/* deps to the package should be enough for TS to find its ambient typedefs

If that's not the case you may be able to use /// directive:

https://www.typescriptlang.org/docs/handbook/triple-slash-directives.html

/// <reference types="@types/gtag.js" />

Our internal code analyzers get very angry when we do stuff like that.

On the Docusaurus repo we only care about providing a setup that TS agree with, not all the analyzers in the world 😅

I removed it

lanegoolsby · 2022-10-03T19:30:11Z

@slorber I'm very confused on why the tests are failing. I didn't touch anything in that package tree. Any ideas what might cause this?

Here's the failing test and line:

docusaurus/packages/stylelint-copyright/src/__tests__/index.test.ts

Line 100 in 53dbbe3

expect(fixedCode).toBe(testCase.fixed);

I was able to narrow down that there are missing carriage returns when feeding the formatted text into the stylelint package. But I didn't touch anything that would have made an impact to how the strings are concatenated?

Some examples of the failing strings:

Test case 1

Expected:

    /**
     * Copyright
     */  
    .foo {}

Result:

    /**
     * Copyright
     */.foo {} <-- no carriage return

Test case 2

Expected:

    /**
     * Copyright
     */
    
    /**
    * Copyright
    */
    
    .foo {}

Result:

    /**
     * Copyright
     *//**  <-- no carriage return
    * Copyright
    */
    
    .foo {}

Josh-Cena · 2022-10-03T19:34:42Z

@lanegoolsby apparently your dependency installation touched a lot more dependencies than it should. Could you try reverting the lockfile changes and re-installing?

slorber

I don't believe we need a useGoogleAnalytics hook, unless you provide a very good reason to have a layer on top of the existing native gtag API.

However I'm willing to add proper TS support through the @types/gtag.js package (which seems not currently setup properly). Do you need help?

packages/docusaurus-plugin-google-gtag/src/gtag.ts

slorber · 2022-10-05T17:23:31Z

packages/docusaurus-plugin-google-gtag/src/types.d.ts

  gtag: (
    command: string,
-    fields: string,
-    params: {
-      page_title?: string;
-      page_location?: string;
-      page_path?: string;
-    },
+    action: string,
+    params?: string | Gtag.EventParams,
  ) => void;


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

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.

slorber · 2022-10-05T17:29:30Z

packages/docusaurus-plugin-google-gtag/src/utils.ts

+ * LICENSE file in the root directory of this source tree.
+ */
+
+export {useGoogleAnalytics} from './utils/useGoogleAnalytics';


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 😅

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 require ts-ignore statements for the window.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 the configure 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 😅).

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:

{ tagName: 'script', innerHTML: ` window.dataLayer = window.dataLayer || []; function gtag(){dataLayer.push(arguments);} gtag('js', new Date()); gtag('config', '${trackingID}', { ${ anonymizeIP ? "'anonymize_ip': true" : '' } });`,

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:

<button onClick={e => { gtag('event', <action>, { 'event_category': <category>, 'event_label': <label>, 'value': <value> }); }}/>

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

slorber · 2022-10-06T09:39:35Z

My suggestion for this PR: add a case in website/_dogfooding where we send a custom event to GA

It should work locally with yarn build:website:fast && yarn serve:website, send events to our GA prod instance, and be typesafe

lanegoolsby · 2022-10-19T01:15:27Z

@slorber I removed the hook for now. I left the modernization of the gtag event process in place since its still relevant.

I still see value for something like this. There are 3 or 4 Community plugins, in addition to the GA and GTag plugins, that are just for adding analytics to a DS site. It would be super nice to have a codified way to send custom events to [insert analytics plugin here] via an official DS API.

packages/docusaurus-plugin-google-gtag/src/types.d.ts

slorber · 2022-10-21T08:56:36Z

I still see value for something like this.

I understand you see the value, but you never explain why. On my side, I see no value and I provided good reasons for that. If you want to convince me, please tell me why it is needed.

There are 3 or 4 Community plugins, in addition to the GA and GTag plugins, that are just for adding analytics to a DS site.

I'm not sure to understand. As far as I know there's no community-plugin for google analytics (at least can't find anything on npm)

Now if you introduce a useGoogleAnalytics hook, this hook is only meant to be used with... google analytics.

I can understand that there might be a value in creating a generic useAnalytics(), so that the same interface can send custom events to multiple analytics systems, but this is not what your PR implements here.

Note that this is the value proposition of a big startup called Segment, for which we have a plugin already: https://github.com/xer0x/docusaurus-plugin-segment

Also related to https://github.com/DavidWells/analytics for which it could be interesting to have a Docusaurus plugin

It would be super nice to have a codified way to send custom events to [insert analytics plugin here] via an official DS API.

The codified way to send custom events to Google Analytics already exists: it is the official gtag API, provided by google. Docusaurus does not provide any additional value if it created an abstraction layer on top of it. Users can use the official API directly to send custom events:

<button onClick={e => {
  window.gtag('event', 'aaa', {
    'event_category' : 'bbb',
    'event_label' : 'ccc'
  });
}}/>

Now if you want to create a custom abstraction for all analytics services, the hook definitively can't be called "useGoogleAnalytics", and there are other libs that focus on solving this problem, like https://github.com/DavidWells/analytics, for which we could build a Docusaurus plugin

slorber · 2022-10-21T11:32:51Z

packages/docusaurus-plugin-google-gtag/src/types.d.ts

-interface Window {
-  gtag: (
-    command: string,
-    fields: string,
-    params: {
-      page_title?: string;
-      page_location?: string;
-      page_path?: string;
-    },
-  ) => void;
-}


note: this type is only used internally, and is not exposed by the gtag package, so it shouldn't affect anyone.

slorber · 2022-10-21T11:37:19Z

website/src/types.d.ts

@@ -6,3 +6,4 @@
 */

 /// <reference types="@docusaurus/plugin-ideal-image" />
+/// <reference types="@types/gtag.js" />


not very satisfied with this setup atm, as it puts the burden to the site owner to reference the appropriate packages, but good enough to move on

What I wish is to have the type auto reference itself if the preset-classic is installed (since it contains the gtag plugin, even though it might be disabled).

I'd like to have references: preset => plugin gtag => @types/gtag.js

For now, if I add such a ref in index.ts, it gets stripped out from the .d.ts file, unfortunately.

@Josh-Cena I'll merge as it it but if you have any idea to make that site ref un-necessary that would be cool.

slorber · 2022-10-21T11:39:39Z

website/_dogfooding/_pages tests/analytics.tsx

+        <button
+          type="button"
+          onClick={() => {
+            window.gtag('event', 'docusaurus-test-event', {


side note: it doesn't work in dev/deploy-preview because we don't load gtag script there

We should probably expose a gtag | undefined instead of just gtag, to make it clear that user has to program defensively against this api

slorber · 2022-10-21T12:32:37Z

Not sure why the CLA bot is unhappy 😅

Lane Goolsby and others added 4 commits September 27, 2022 08:24

Adding GA hook. Need help exporting it.

2382178

Fixed and working

9ea3476

Merge branch 'facebook:main' into main

e3b9137

Derp

cda7b8a

lanegoolsby requested review from slorber, lex111 and Josh-Cena as code owners September 28, 2022 20:48

facebook-github-bot added the CLA Signed Signed Facebook CLA label Sep 28, 2022

slorber reviewed Sep 29, 2022

View reviewed changes

slorber self-assigned this Sep 30, 2022

slorber marked this pull request as draft September 30, 2022 14:00

Lane Goolsby added 2 commits October 3, 2022 10:14

PR Feedback

961966e

Getting rid of local registry

b4780e2

Adding if case for when in develop mode

1631f43

Lane Goolsby added 2 commits October 3, 2022 13:05

Reverting yarn.lock to fix tests

1bd3edf

Auto-format to kick a new build

bd0683e

lanegoolsby marked this pull request as ready for review October 4, 2022 13:51

slorber changed the title ~~feat (plugin-gtag): Adding useGoogleAnalytics hook~~ feat(plugin-gtag): Adding useGoogleAnalytics hook Oct 5, 2022

slorber requested changes Oct 5, 2022

View reviewed changes

Lane Goolsby added 2 commits October 18, 2022 18:01

Backing out abstraction

88e9407

Derp

6d49f61

Removing a somewhat useless comment in order to kick the build

3cb8773

lanegoolsby changed the title ~~feat(plugin-gtag): Adding useGoogleAnalytics hook~~ chore (plugin-gtag): Updating GTag plugin to modern SPA recommendations Oct 19, 2022

slorber reviewed Oct 21, 2022

View reviewed changes

packages/docusaurus-plugin-google-gtag/src/types.d.ts Outdated Show resolved Hide resolved

Update packages/docusaurus-plugin-google-gtag/src/types.d.ts

8ce5d72

slorber changed the title ~~chore (plugin-gtag): Updating GTag plugin to modern SPA recommendations~~ refactor(plugin-gtag): update gtag plugin to modern SPA recommendations Oct 21, 2022

slorber added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Oct 21, 2022

better google analytics types integration

be36601

slorber approved these changes Oct 21, 2022

View reviewed changes

slorber reviewed Oct 21, 2022

View reviewed changes

slorber marked this pull request as draft October 21, 2022 12:58

slorber marked this pull request as ready for review October 21, 2022 12:58

slorber merged commit e411332 into facebook:main Oct 21, 2022

slorber added to backport This PR is planned to be backported to a stable version of Docusaurus and removed to backport This PR is planned to be backported to a stable version of Docusaurus labels Oct 21, 2022

This was referenced Oct 19, 2023

chore: v3.0.0-rc.0 release #9418

Merged

chore: v3.0.0-rc.1 release #9453

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(plugin-gtag): update gtag plugin to modern SPA recommendations #8143

refactor(plugin-gtag): update gtag plugin to modern SPA recommendations #8143

lanegoolsby commented Sep 28, 2022

netlify bot commented Sep 28, 2022 •

edited

Loading

github-actions bot commented Sep 28, 2022 •

edited

Loading

slorber Sep 29, 2022

lanegoolsby Sep 29, 2022

slorber Sep 30, 2022

lanegoolsby Sep 30, 2022

lanegoolsby Sep 30, 2022

slorber Sep 30, 2022

lanegoolsby Oct 19, 2022

lanegoolsby commented Oct 3, 2022

Josh-Cena commented Oct 3, 2022

slorber left a comment

slorber Oct 5, 2022

lanegoolsby Oct 5, 2022 •

edited

Loading

slorber Oct 5, 2022 •

edited

Loading

lanegoolsby Oct 5, 2022

slorber Oct 6, 2022 •

edited

Loading

slorber commented Oct 6, 2022

lanegoolsby commented Oct 19, 2022

slorber commented Oct 21, 2022 •

edited

Loading

slorber Oct 21, 2022

slorber Oct 21, 2022

slorber Oct 21, 2022

slorber commented Oct 21, 2022

refactor(plugin-gtag): update gtag plugin to modern SPA recommendations #8143

refactor(plugin-gtag): update gtag plugin to modern SPA recommendations #8143

Conversation

lanegoolsby commented Sep 28, 2022

Pre-flight checklist

Motivation

Test Plan

Test links

Related issues/PRs

netlify bot commented Sep 28, 2022 • edited Loading

✅ [V2]

github-actions bot commented Sep 28, 2022 • edited Loading

⚡️ Lighthouse report for the deploy preview of this PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lanegoolsby commented Oct 3, 2022

Test case 1

Test case 2

Josh-Cena commented Oct 3, 2022

slorber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lanegoolsby Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

slorber Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slorber Oct 6, 2022 • edited Loading

Choose a reason for hiding this comment

slorber commented Oct 6, 2022

lanegoolsby commented Oct 19, 2022

slorber commented Oct 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slorber commented Oct 21, 2022

netlify bot commented Sep 28, 2022 •

edited

Loading

github-actions bot commented Sep 28, 2022 •

edited

Loading

lanegoolsby Oct 5, 2022 •

edited

Loading

slorber Oct 5, 2022 •

edited

Loading

slorber Oct 6, 2022 •

edited

Loading

slorber commented Oct 21, 2022 •

edited

Loading