Adding <GoogleTagManager> component to @next/third-parties #56106
Conversation
ijjk
added
created-by: Chrome Aurora
|
Just saw a minor typo in the heading 🫣😅
|
janicklas-ralph
changed the title
janicklas-ralph
requested review from
timneutkens,
ijjk,
shuding,
huozhi,
feedthejim,
ztanner and
wyattjoh
as code owners
| @@ -1,2 +1,3 @@ | |||
| export { default as GoogleMapsEmbed } from './GoogleMapsEmbed' | |||
| export { default as YouTubeEmbed } from './YouTubeEmbed' | |||
| export { default as useGoogleTagManager } from './useGoogleTagManager' | |||
There was a problem hiding this comment.
@janicklas-ralph Do you know if the bundle is being tree-shaken with this approach? For example, I import useGoogleTagManager in my app, is GoogleMapsEmbed and YouTubeEmbed included into the total bundle even if they're not used?
| ) | ||
| } | ||
|
|
||
| export default Page |
There was a problem hiding this comment.
We can do this in a separate PR, but we should update the GTM example in the examples/ repo to show this newer approach
There was a problem hiding this comment.
Is the double <GoogleTagManager gtmId="GTM-XYZ" /> on line 13 and 18 an oversight? Same in test/e2e/third-parties/pages/gtm.js
There was a problem hiding this comment.
I had thought the same in the beginning, but I recall @janicklas-ralph mentioning that it is was deliberate to test that the behavior if an identical GTM containers is included by accident
@janicklas-ralph Can you clarify, and if so, add a note here in a future PR? (not urgent)
Stats from current PRDefault BuildGeneral
Client Bundles (main, webpack)
Legacy Client Bundles (polyfills)
Client Pages
Client Build Manifests
Rendered Page Sizes
Edge SSR bundle Size
Middleware size
|
| let currDataLayerName = 'dataLayer' | ||
|
|
||
| export function GoogleTagManager(props: GTMParams) { | ||
| const { gtmId, dataLayerName = 'dataLayer', auth, preview, dataLayer } = props |
There was a problem hiding this comment.
Can we rename gtmId to just id?
There was a problem hiding this comment.
I originally had it as id but since I made it into a component, I am accepting the GTM params as props
<GoogleTagManager gtmId="GTM_XYZ" />
In this case id might clash with HTML id so I changed it to gtmID
| export function GoogleTagManager(props: GTMParams) { | ||
| const { gtmId, dataLayerName = 'dataLayer', auth, preview, dataLayer } = props | ||
|
|
||
| currDataLayerName = dataLayerName |
There was a problem hiding this comment.
I don't think this pattern is edge-case free. When you render this component it will use the initial dataLayer prop to instantiate the script by id. Then afterwards any changes to the dataLayer prop will not be reflected in teh gtm script but will alter this module scoped variable.
The sendGTMEvent refers to the current dataLayer rather than the the one initially used instantiate the script so a wayward props update could breat gtm tracking.
If it is true that you can't hot-update the datalayer in GTM (meaning you need to just pick it at the start of the app runtime and never change it) then we should align the module global to only update when the gtm script will actually run which is just once one hte first render.
There was a problem hiding this comment.
That makes sense! I have updated the code to only modify currDataLayerName during initialization
| if (currDataLayerName === undefined) { | ||
| currDataLayerName = dataLayerName | ||
| } |
There was a problem hiding this comment.
Consider adding a warning in dev if you later update the layer name to something else. Since you cannot modify the layer name after initialization this is erroneous but since the default layer continues to work I'd just make it a console error since it'll just work with the initial layer in production
janicklas-ralph
changed the title
This PR adds the
<GoogleTagManager>component along with thesendGTMEventhelper to@next/third-partiesrepo.