refactor: simplify GTM logic + unmount on permission removal #1047

iamacook · 2022-11-03T11:42:16Z

What it solves

Refactors GTM and adds disale functionality.

How this PR fixes it

TagManager.initialize and TagManager.dataLayer have been simplified.
GA cookies are removed and the session is reloaded on TagManager,disable
Logging now reflects what is being sent to GTM.
An unnecessary isGtmLoaded flag was also removed.

How to test it

Refactor

GTM should initialise and events should be sent as before. (make sure the intial pageview works especially)
No errors/warnings should be shown in the console if the cookie preferences are changed, A reload should occur when revoking permission.
When disabling "Analytics" preferences, the GTM tag should be removed. (no tracking visible in Network tab)

Analytics changes

The GTM scripts are all removed on revocation.

cloudflare-workers-and-pages · 2022-11-03T11:44:21Z

Deploying with Cloudflare Pages

Latest commit:	`284f791`
Status:	✅ Deploy successful!
Preview URL:	https://93729feb.web-core.pages.dev
Branch Preview URL:	https://refactor-gtm.web-core.pages.dev

View logs

github-actions · 2022-11-03T11:45:32Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	0	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

katspaugh

Left suggestions in the code to allow stacking pre-init events as you had in another PR.

src/services/analytics/TagManager.ts

iamacook · 2022-11-04T07:02:04Z

Left suggestions in the code to allow stacking pre-init events as you had in another PR.

We want to discuss event stacking with legal as the team is concerned regarding consent.

katspaugh

Looks good!
Let's merge after 1.0.4, tho.

iamacook · 2022-11-04T10:18:08Z

We want to discuss event stacking with legal as the team is concerned regarding consent.

Just for future reference: @johannesmoormann confirmed that we don't want to stack event prior to cookie agreement.

github-actions · 2022-11-07T16:38:16Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	0	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

francovenica · 2022-11-10T03:43:11Z

If I enable the "Analytic" checkbox, track a few actions and then I disable the checkbox this error start to show in the console for every action.

The error goes away once I reload the page manually

iamacook · 2022-11-10T10:08:04Z

If I enable the "Analytic" checkbox, track a few actions and then I disable the checkbox this error start to show in the console for every action.

Turns out GTM embeds several scripts of similar names:

I've updated the code and it now removes them all. No error should be shown in the console anymore.

francovenica · 2022-11-10T12:50:05Z

Tried this again and I can still see the same issue. Can u check if the PR updated correctly?

iamacook · 2022-11-10T14:18:04Z

Tried this again and I can still see the same issue. Can u check if the PR updated correctly?

Despite unmounting the scripts, they remain in memory. @usame-algan and I will investigate this further.

src/config/constants.ts

github-actions · 2022-11-16T12:48:49Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	0	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

usame-algan · 2022-11-21T13:01:29Z

src/services/analytics/TagManager.ts

+  isInitialized: () => {
+    const GTM_SCRIPT = 'https://www.googletagmanager.com/gtm.js'

-export const _getGtmDataLayerScript = (dataLayer: DataLayer) => {
-  const script = document.createElement('script')
+    return !!document.querySelector(`[src^="${GTM_SCRIPT}"]`)


Would it work if we checked that window.dataLayer exists instead? It would be a bit simpler than querying the DOM.

I would sooner check the DOM. We could manually push to window.dataLayer and then this would return true when GTM isn't initialised.

usame-algan

Great idea to reload the page! 💯

francovenica · 2022-11-22T12:47:06Z

I agree with the solution. I don't think people are enabling/disabling the analytics every 5 mins for the reload to be an inconvenience.
LGTM

github-actions · 2022-11-22T12:52:27Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	0	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

github-actions · 2022-11-22T12:56:00Z

Branch preview

https://pr1047--webcore.review-react-br.5afe.dev

iamacook added 3 commits November 3, 2022 10:07

feat: add TagManager.destroy function

4c4f9bb

refactor: GTM

c0c39fb

fix: add dev log

55adb40

iamacook self-assigned this Nov 3, 2022

This was referenced Nov 3, 2022

refactor: GTM #1033

Closed

feat: add TagManager.destroy functionilty #1043

Closed

iamacook requested a review from usame-algan November 3, 2022 11:43

Merge branch 'dev' into refactor-gtm

d3ca65d

Merge branch 'dev' into refactor-gtm

a86cfc9

iamacook changed the title ~~refactor: GTM~~ refactor: simplify GTM logic + unmount on permission removal Nov 3, 2022

fix: extract constant

732989e

katspaugh reviewed Nov 4, 2022

View reviewed changes

src/services/analytics/TagManager.ts Outdated Show resolved Hide resolved

src/services/analytics/TagManager.ts Outdated Show resolved Hide resolved

katspaugh approved these changes Nov 4, 2022

View reviewed changes

iamacook changed the title ~~refactor: simplify GTM logic + unmount on permission removal~~ [Merge AFTER 1.0.4] refactor: simplify GTM logic + unmount on permission removal Nov 4, 2022

Merge branch 'dev' into refactor-gtm

266ed6a

iamacook changed the title ~~[Merge AFTER 1.0.4] refactor: simplify GTM logic + unmount on permission removal~~ refactor: simplify GTM logic + unmount on permission removal Nov 9, 2022

fix: remove all GTM scripts

d0db466

iamacook marked this pull request as draft November 10, 2022 14:18

fix: disable GTM/GA programmatically

0c84c2c

iamacook commented Nov 11, 2022

View reviewed changes

src/config/constants.ts Outdated Show resolved Hide resolved

Merge branch 'dev' into refactor-gtm

dbefb6d

iamacook added 6 commits November 18, 2022 10:56

Merge branch 'dev' into refactor-gtm

2bc7cee

fix: remove cookies

63c372e

fix: adjust tests

9822f0a

fix: reload instead of using flag

178e172

fix: tests

c0ca8bc

Merge branch 'dev' into refactor-gtm

284f791

iamacook marked this pull request as ready for review November 20, 2022 15:51

usame-algan reviewed Nov 21, 2022

View reviewed changes

usame-algan approved these changes Nov 21, 2022

View reviewed changes

Merge branch 'dev' into refactor-gtm

c156d97

iamacook merged commit 0e36eb2 into dev Nov 22, 2022

iamacook deleted the refactor-gtm branch November 22, 2022 13:37

katspaugh mentioned this pull request Nov 28, 2022

Release v1.1.0 #1255

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: simplify GTM logic + unmount on permission removal #1047

refactor: simplify GTM logic + unmount on permission removal #1047

iamacook commented Nov 3, 2022 •

edited

Loading

cloudflare-workers-and-pages bot commented Nov 3, 2022 •

edited

Loading

github-actions bot commented Nov 3, 2022 •

edited

Loading

katspaugh left a comment

iamacook commented Nov 4, 2022

katspaugh left a comment

iamacook commented Nov 4, 2022

github-actions bot commented Nov 7, 2022 •

edited

Loading

francovenica commented Nov 10, 2022

iamacook commented Nov 10, 2022

francovenica commented Nov 10, 2022

iamacook commented Nov 10, 2022

github-actions bot commented Nov 16, 2022 •

edited

Loading

usame-algan Nov 21, 2022

iamacook Nov 21, 2022

usame-algan left a comment

francovenica commented Nov 22, 2022

github-actions bot commented Nov 22, 2022

github-actions bot commented Nov 22, 2022

refactor: simplify GTM logic + unmount on permission removal #1047

refactor: simplify GTM logic + unmount on permission removal #1047

Conversation

iamacook commented Nov 3, 2022 • edited Loading

What it solves

How this PR fixes it

How to test it

Refactor

Analytics changes

cloudflare-workers-and-pages bot commented Nov 3, 2022 • edited Loading

Deploying with Cloudflare Pages

github-actions bot commented Nov 3, 2022 • edited Loading

ESLint Summary View Full Report

katspaugh left a comment

Choose a reason for hiding this comment

iamacook commented Nov 4, 2022

katspaugh left a comment

Choose a reason for hiding this comment

iamacook commented Nov 4, 2022

github-actions bot commented Nov 7, 2022 • edited Loading

ESLint Summary View Full Report

francovenica commented Nov 10, 2022

iamacook commented Nov 10, 2022

francovenica commented Nov 10, 2022

iamacook commented Nov 10, 2022

github-actions bot commented Nov 16, 2022 • edited Loading

ESLint Summary View Full Report

usame-algan Nov 21, 2022

Choose a reason for hiding this comment

iamacook Nov 21, 2022

Choose a reason for hiding this comment

usame-algan left a comment

Choose a reason for hiding this comment

francovenica commented Nov 22, 2022

github-actions bot commented Nov 22, 2022

ESLint Summary View Full Report

github-actions bot commented Nov 22, 2022

Branch preview

iamacook commented Nov 3, 2022 •

edited

Loading

cloudflare-workers-and-pages bot commented Nov 3, 2022 •

edited

Loading

github-actions bot commented Nov 3, 2022 •

edited

Loading

github-actions bot commented Nov 7, 2022 •

edited

Loading

github-actions bot commented Nov 16, 2022 •

edited

Loading