Implement a very simple conditional templating system

[felixarntz]

In regards to https://github.com/GoogleChromeLabs/third-party-capital/pull/71#issuecomment-2332572666: This is a very simplistic (!) attempt at supporting conditionals in the templating system.

It allows using Mustache-like conditionals, e.g.

{{#consentValues}}window['gtag-'+{{l}}]('consent', {{consentType}}, {{consentValues}});{{/consentValues}}

We could use that to enhance the solution from #71, making consentValues null by default, so that the call is only made if this is specified. In light of not supporting more complicated conditionals, it would make sense to leave "default" as the default value for consentType, as the more important piece of information to provide for such calls is the configuration object anyway.

It probably has several limitations e.g. when it comes to having multiple conditional clauses of the same name in one code snippet etc., but as long as we're aware of that limitation, maybe this is an okay start for now? There are certainly more reliable and established solutions out there, but for those it would probably make more sense to pull in a library, which requires more discussion. So maybe this works? :)

(My first JS PR in this codebase, so let's see if I got things right.)

[felixarntz]

@huang-julien @flashdesignory Note that there's a PHPUnit test failure, which however is unrelated to the new code templating logic: For some reason, the new consentType and consentValues values are included as query parameters alongside the https://www.googletagmanager.com/gtag/js URL, which is clearly wrong (they should only affect the code).

Have you been able to verify that that is not the case on the JS side? Potentially it's something that I implemented incorrectly in PHP, but when comparing with the JS code, it seems similar. Since there is no test coverage for the exact third-party outputs on the JS side, I just want to make sure this isn't broken on the JS side as well.

[huang-julien]

hmmm the JSON data structure should prevent that. A Script object can't have url and code at the same time.

You're probably merging all optionnal param into a same object :eyes:

  {
      "url": "https://www.googletagmanager.com/gtag/js",
      "params": ["id"],
      "optionalParams": {
        "l": null
      },
      "strategy": "worker",
      "location": "head",
      "action": "append",
      "key": "gtag"
    },
    {
      "code": "window[{{l}}]=window[{{l}}]||[];window['gtag-'+{{l}}]=function (){window[{{l}}].push(arguments);};window['gtag-'+{{l}}]('consent', {{consentType}}, {{consentValues}});window['gtag-'+{{l}}]('js',new Date());window['gtag-'+{{l}}]('config',{{id}})",
      "params": ["id"],
      "optionalParams": {
        "l": "dataLayer",
        "consentType": "default",
        "consentValues": {
          "ad_user_data": "denied",
          "ad_personalization": "denied",
          "ad_storage": "denied",
          "analytics_storage": "denied",
          "wait_for_update": 500
        }
      },
      "strategy": "worker",
      "location": "head",
      "action": "append",
      "key": "setup"
    }

[felixarntz]

@huang-julien Makes sense in general. But see this code: https://github.com/GoogleChromeLabs/third-party-capital/blob/main/src/utils/index.ts#L106-L113

As far as I can tell, this combines all params and optional params from all scripts that are part of the third party into one array. And further down that is passed to formatUrl. Maybe I misread that code somewhere, but I would think that based on that logic the JS side has the same bug at the moment.

[felixarntz]

@housseindjirdeh @flashdesignory It would be great to get your input on https://github.com/GoogleChromeLabs/third-party-capital/pull/73#issuecomment-2332709312 - this may be an already present bug that's unrelated to this PR, but is "exposed" by the PHPUnit tests?

[huang-julien]

tbh, we don't really use formatData with nuxt script since we need to do our own transformation.

With JS formatData, it filters all possible params into an array then applies the external's script default value if necessary (that's maybe not intended but shouldn't cause a bug). formatUrl has a last optionalParams parameters which is where we set it if not present here https://github.com/GoogleChromeLabs/third-party-capital/blob/ad7e126cb1bd64b2db76cd1e2c88b7a3f9d919ac/src/utils/index.ts#L163 and https://github.com/GoogleChromeLabs/third-party-capital/blob/ad7e126cb1bd64b2db76cd1e2c88b7a3f9d919ac/src/utils/index.ts#L171

[flashdesignory]

@huang-julien Makes sense in general. But see this code: https://github.com/GoogleChromeLabs/third-party-capital/blob/main/src/utils/index.ts#L106-L113

As far as I can tell, this combines all params and optional params from all scripts that are part of the third party into one array. And further down that is passed to formatUrl. Maybe I misread that code somewhere, but I would think that based on that logic the JS side has the same bug at the moment.

I'll take a look on Monday. This is not really introducing a bug at the moment, but it's not the cleanest way to handle both params and optionalParams.

[felixarntz]

I'll take a look on Monday. This is not really introducing a bug at the moment, but it's not the cleanest way to handle both params and optionalParams.

Are you sure? From what I can tell, it is introducing a bug as of #71. The code is combining the available params and optionalParams, so far so good. But I think the problem is that it combines those params from all scripts, when this should rather be happening per script.

The consentType and consentValues keys are only relevant for the Analytics code script, not the Analytics url script. But since all params for all scripts are merged, this distinction is ignored, leading to code specific parameters to be included in the URL. I think that's the bug - we only haven't encountered it before since until #71 the params and optionalParams for both scripts always matched.

[flashdesignory]

@felixarntz - found the issue and how to solve it.. just need some time to actually implement it. should have something tomorrow morning the latest.

[felixarntz]

@flashdesignory Once #74 is merged, I'll iterate on this so we can hopefully get it in soon and that way unblock #71 and #72, if you're on board with this approach?

[huang-julien]

Should we add a default value for consentType (in this PR or in another one) ? We can indicate the default value with some JSDoc

[felixarntz]

Should we add a default value for consentType (in this PR or in another one) ? We can indicate the default value with some JSDoc

Isn't the default values for consentType already set to "default"?

[huang-julien]

oops my bad, was doing some tests and forogt to add it 😅 sorry . Anyway, it's working well for me :+1: