GoogleChromeLabs / third-party-capital

A library that provides best practices for loading popular third-parties
Apache License 2.0
117 stars 8 forks source link

fix(ga): setup default consent state #71

Closed huang-julien closed 2 months ago

huang-julien commented 2 months ago

https://github.com/nuxt/scripts/issues/243

Hey :wave: this PR set the consent mode to false by default in GA.

https://developers.google.com/tag-platform/security/guides/consent?consentmode=basic#gtag.js

output with nuxt-scripts when denied by default

image

output with nuxt-script when we did update the consent mode

image

felixarntz commented 2 months ago

@housseindjirdeh @flashdesignory I realize this was merged, so I'm late to the party, but what about https://github.com/GoogleChromeLabs/third-party-capital/pull/72#discussion_r1744543990? Has this been considered here?

In the linked docs, it says:

By default, no consent mode values are set.

This makes me concerned about having denied in here by default.

housseindjirdeh commented 2 months ago

@felixarntz Hmm that's a good point. I had thought that denied was the default state regardless of whether a value is passed, but I agree that's not clear from the documentation.

@huang-julien Could you submit a quick patch to make sure no values are passed in unless provided by the user?

felixarntz commented 2 months ago

@housseindjirdeh @huang-julien I think we would need to be able to support conditional logic in the templating system, or not? Since right now we're just replacing {{key}} with actual strings, this may require some more thought - or maybe there's a straightforward solution?

housseindjirdeh commented 2 months ago

Yeah I left a comment in the other PR about this: https://github.com/GoogleChromeLabs/third-party-capital/pull/72#discussion_r1745939548

My thinking is maybe we can pass in null values as a starting point? If we want to support conditional logic, we'll have to think about it a bit more 🤔

flashdesignory commented 2 months ago

I would approach it similar to the datalayer name ( 'l' ) option. If it's null, we don't assign it. On the JS side, that's done in the formatData function I believe, or somewhere in the utils file where we parse out nullish values.

flashdesignory commented 2 months ago

see here: https://github.com/GoogleChromeLabs/third-party-capital/blob/main/src/utils/index.ts#L43

housseindjirdeh commented 2 months ago

I would approach it similar to the datalayer name ( 'l' ) option. If it's null, we don't assign it. On the JS side, that's done in the formatData function I believe, or somewhere in the utils file where we parse out nullish values.

@flashdesignory If we can do that, that would be great. @huang-julien @felixarntz Wdyt about applying similar logic to the formatCode function?

huang-julien commented 2 months ago

The only issue i see is that we also have params within the content. We'd probably need to format the optionnalparams too

huang-julien commented 2 months ago

That won't do, the replacement system stringifies the input. This means that code injection don't work.

I feel like we're reaching the limit of what we can do with JSON data. We should probably start to move away from it ? There no pros for us to keep JSON in the repo as we're already converting it to JS object with rollup in the final bundle.

felixarntz commented 2 months ago

I don't think we'd need code injection. But we'd need a templating system that allows things like conditionals and loops. There are many such templating systems out there (e.g. https://mustache.github.io/), so I wonder whether any of those would make sense to use here. Though we would probably want to find something that's very lightweight, and I'm not sure what our options are in that regard.

Alternatively, we come up with a simplistic custom implementation for something like {{#consent}}...{{/consent}} that would ensure the code in between the clause is not included if the respective parameter is empty. While that would be sort of reimplementing some of those templating systems, if we do it in a "quick and dirty" way, we'd still achieve what we want without including a dependency that could notably increase bundle size.

Regarding removal of JSON, note that the repository also includes a PHP implementation. Using JSON is an elegant way to share the configuration in a way that's language agnostic.

flashdesignory commented 2 months ago

@huang-julien - just a heads up, as you noticed comments from Felix ... This repo supports a JavaScript and PHP integration. We have to be mindful with our choices to ensure that we can support both languages. This might not be the most ideal solution always for JavaScript, but it's a necessity from our side to ensure both languages are staying in sync from a functionality perspective.

The great thing about having more integrations is that we have more engineers to collaborate with and I think that's a powerful side effect.

huang-julien commented 2 months ago

Oh i see that make sense 😅

felixarntz commented 2 months ago

@huang-julien @flashdesignory I just gave implementing a very simple conditional template system a shot in #73. Might work just okay for our purposes.

If we want to proceed with this, I can add the same logic for the PHP side in that PR, and then apply it to the gtag snippet handling.