fix(gtm): don't declare dataLayer on the global Window - #855
Open
Togetic wants to merge 1 commit into
Open
Conversation
The GTM registry augments the global `Window` with the whole
`GoogleTagManagerApi`, which includes a required
`dataLayer: DataLayer & { push: DataLayerPush }`.
That makes this package irreconcilable with any other package declaring
`Window.dataLayer`. The common case is `@gtm-support/core` (used by
`@gtm-support/vue-gtm`), which declares it as optional:
declare global { interface Window { dataLayer?: DataLayerObject[] } }
The two cannot merge, so the merged `Window` fails its own
`extends GoogleTagManagerApi` check and every consumer `Window`
augmentation reports TS2430. It is not suppressable from consumer code.
`tsc` 5.9.3 does not verify merged interfaces against their bases, so it
stays silent. TypeScript 7 (tsgo) does check, and reports it — which this
repo will hit itself once nuxt#827 lands.
Declaring `dataLayer` globally is also inaccurate independently of the
conflict: the dataLayer name is configurable via the `l` / `dataLayer`
options, so `window.dataLayer` is not guaranteed to exist. The registry
never relies on the global declaration for it either — it reads
`(window as any)[dataLayerName]` and casts. `window.google_tag_manager`
IS read directly, so that member stays declared.
`GoogleTagManagerApi` itself is unchanged, so
`useScriptGoogleTagManager<T extends GoogleTagManagerApi>()` and the
`use()` return type keep their existing shape. Consumers should read the
dataLayer through that typed proxy, which is also the only access path
that respects a custom dataLayer name.
Contributor
|
@Togetic is attempting to deploy a commit to the Nuxt Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe global Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
commit: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #852.
Problem
The GTM registry augments the global
Windowwith the entireGoogleTagManagerApi:This makes
@nuxt/scriptsirreconcilable with any other package that declaresWindow.dataLayer. The common case is@gtm-support/core— the engine behind@gtm-support/vue-gtm, which a lot of Nuxt/Vue apps still use for GTM:The two declarations merge into one
Window, which then fails its ownextends GoogleTagManagerApicheck. The result isTS2430reported at every one of the consumer's ownWindowaugmentations — nowhere near the actual cause, and not suppressable from consumer code, since neither declaration belongs to them.tsc5.9.3 never verifies merged interfaces against their bases, so it stays silent. TypeScript 7 (tsgo) does check, and reports it — so this repo will hit it itself once #827 lands.The obvious fix does not work
#852 proposed making
dataLayeroptional. I checked, and that is not sufficient — there are two independent incompatibilities stacked, and making it optional only reveals the second:Array<T>.pushis(...items: T[]) => number, whileDataLayerPush's first overload begins(command: string, ...). No amount of optionality reconciles that with a foreign element type. Any fix that keepsdataLayerin the global augmentation stays broken.Repro
Four files,
strict,skipLibCheck— one file per package plus a consumer that augmentsWindow(as most apps do):files
plus
nuxt-scripts.d.tscarrying the registry's type block.mainTS2430dataLayermade optional (#852's proposal)TS2430The fix
Declare only the member that is actually global and actually unambiguous:
Why this is safe
GoogleTagManagerApiis untouched, souseScriptGoogleTagManager<T extends GoogleTagManagerApi>()and theuse()return type keep their exact shape. Anyone readingdataLayeroff the returned proxy is unaffected.dataLayer. It reads(window as any)[dataLayerName]and casts — because the name is configurable.window.google_tag_manageris read directly (use()), which is why that member stays declared.dataLayertyping. The fourwindow.dataLayeroccurrences are all insideinnerHTMLtemplate strings (playground/.../unhead.vue,scripts/generate-sizes.ts), prose in an example (examples/regional-consent/app.vue), or already(window as any)(test/nuxt-runtime/consent-default.nuxt.test.ts).l/dataLayeroptions, sowindow.dataLayeris not guaranteed to exist; the typed proxy is the only access path that respects a custom name.Note on scope
This is a type-level change only — zero runtime bytes.
It is technically breaking for anyone who reads
window.dataLayerdirectly and relies on this package to type it. That was arguably never a promise this package should have made (the name is configurable), and such code keeps working with a one-line local augmentation or by using theuseScriptGoogleTagManager()proxy. Happy to put it behind a major, or to add the narrowerWindow { dataLayer?: unknown }shim instead, if you would prefer — whichever you would rather land.I verified the above with the minimal reproduction (exact declarations from both packages, TS 7.0.2 and 5.9.3); I have not run the full repo test suite locally.