Skip to content
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

remove window.gtag function #54

Merged
merged 7 commits into from
Jul 24, 2024

Conversation

flashdesignory
Copy link
Collaborator

should resolve #53

@kara

Copy link
Contributor

@huang-julien huang-julien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to remove it from types

@flashdesignory
Copy link
Collaborator Author

We also need to remove it from types

duh, will do 🤦

src/types/type-declarations.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@housseindjirdeh housseindjirdeh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few questions

src/third-parties/google-analytics/data.json Show resolved Hide resolved
src/types/type-declarations.ts Show resolved Hide resolved
@@ -75,14 +79,15 @@ export interface GTag {
(fn: 'config', opt: 'reset'): void;
}

export interface GoogleAnalyticsApi {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is technically a breaking change for the types

it's probably safe as i can't imagine anyone besides Nuxt Scripts using it but please be mindful of these changes down the line

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct, Nuxt Script is currently the only framework using the types from here

"strategy": "worker",
"location": "head",
"action": "append",
"key": "gtag"
},
{
"code": "window.dataLayers=window.dataLayers||{};window.dataLayers['{{dataLayerName}}']=window.dataLayers['{{dataLayerName}}']||[];window.gtag=function gtag(){window.dataLayers['{{dataLayerName}}'].push(arguments);};window.gtag('js',new Date());window.gtag('config','{{id}}')",
"params": ["id", "dataLayerName"],
"code": "window['{{l}}']=window['{{l}}']||[];window['{{l}}'].push({'js':new Date()});window['{{l}}'].push({'config':'{{id}}'})",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is also a breaking change

@harlan-zw
Copy link

happy to have this merged as is but we definitely want to avoid breaking changes like this in the future between patch / minors

Copy link
Contributor

@huang-julien huang-julien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this will probably break other integrations depending on tpc. We shouldn't really have this issue in nuxt-script imo

@flashdesignory
Copy link
Collaborator Author

happy to have this merged as is but we definitely want to avoid breaking changes like this in the future between patch / minors

Absolutely, breaking changes are possible here, since we're still implementing it and no other framework depends on these changes at the moment.

@flashdesignory flashdesignory merged commit d39b6a0 into GoogleChromeLabs:main Jul 24, 2024
2 checks passed
@flashdesignory flashdesignory deleted the thor/gtag-02 branch July 24, 2024 15:11
@harlan-zw
Copy link

harlan-zw commented Jul 24, 2024

happy to have this merged as is but we definitely want to avoid breaking changes like this in the future between patch / minors

Absolutely, breaking changes are possible here, since we're still implementing it and no other framework depends on these changes at the moment.

The issue is that you have versioned the package as stable so package managers will update end users' versions whenever you do a patch/minor as third-party-capital is a dependency of Nuxt Scripts.

We can lock the TPC version though as a workaround on our end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gtag() is getting overriden when using multiple gtm or ga with different dataLayers
4 participants