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

fix: add optionalPrams properties in JSON data #59

Merged
merged 7 commits into from
Jul 27, 2024

Conversation

huang-julien
Copy link
Contributor

@huang-julien huang-julien commented Jul 25, 2024

fix #58

Hey 👋 this PR move optional params into an optionalParam array. It also adds default values to code.

This should allow integrations to avoid adding default values that can be handled by tpc or google scripts.

@huang-julien
Copy link
Contributor Author

huang-julien commented Jul 25, 2024

@huang-julien
Copy link
Contributor Author

huang-julien commented Jul 25, 2024

This PR does not introduce any breaking change. But in a future major version, optional params should be removed from param

@huang-julien huang-julien changed the title fix: datalayer optional param fix: add optional param properties in JSON data Jul 25, 2024
@harlan-zw
Copy link

Can we please consider moving away from json, this template is getting crazy.

@flashdesignory
Copy link
Collaborator

I'm still struggling with the conditional in the code block, especially since the optional param is wrapped in strings, which would result in 'undefined'.

Do you all think, we could give optional params a default value? Maybe something like this?

"code": "window['{{l}}']=window['{{l}}']||[];window['{{l}}'].push({'js':new Date()});window['{{l}}'].push({'config':'{{id}}'})",
"params": ["id"],
"optionalParams": [{"l": "dataLayer"}],

The code block has to get transformed somewhere, correct?
our internal formatCode function could look like this (loosely typed):

export function formatCode(code: string, args?: Inputs, optionalParams?: any[]) {
  return code.replace(/{{(.*?)}}/g, (match) => {
    const key = match.split(/{{|}}/).filter(Boolean)[0];
    const value = args?.[key] ?? optionalParams?.find(entry => entry[key])?.[key];
    return value;
  });
}

@huang-julien huang-julien marked this pull request as draft July 26, 2024 19:13
@huang-julien
Copy link
Contributor Author

yeah indeed, let's stringify the input at code replacement time then. This way, integrations relying on runtime code won't break

@huang-julien
Copy link
Contributor Author

#59 (comment)

wouldn't that be a breaking change if some users are using formatCode since this is exported by the package ?

@housseindjirdeh
Copy link
Collaborator

Can we please consider moving away from json, this template is getting crazy.

I think this warrants a larger discussion but I'm beginning to agree. I'll start a Discord thread about this and maybe we can figure out a cleaner way to do all of this.

@huang-julien huang-julien changed the title fix: add optional param properties in JSON data fix: add optionalPrams properties in JSON data Jul 26, 2024
@huang-julien huang-julien marked this pull request as ready for review July 26, 2024 22:59
@flashdesignory
Copy link
Collaborator

flashdesignory commented Jul 27, 2024

couple additions needed in the index.ts file (filter for // add this lines)

export function formatData(data: Data, args: Inputs): Output {
  const allScriptParams = data.scripts?.reduce(
    (acc, script) => [
        ...acc,
        ...(Array.isArray(script.params) ? script.params : []),
        ...(script.optionalParams ? Object.keys(script.optionalParams) : []) // add this
      ],
    [] as string[],
  );


  // First, find all input arguments that map to parameters passed to script URLs
  const scriptUrlParamInputs = filterArgs(args, allScriptParams);

  // Second, find all input arguments that map to parameters passed to the HTML src attribute
  const htmlUrlParamInputs = filterArgs(
    args,
    data.html?.attributes.src?.params,
  );

  // Third, find the input argument that maps to the slug parameter passed to the HTML src attribute if present
  const htmlSlugParamInput = filterArgs(args, [
    data.html?.attributes.src?.slugParam!,
  ]);

  // Lastly, all remaining arguments are forwarded as separate HTML attributes
  const htmlAttrInputs = filterArgs(
    args,
    [
      ...Object.keys(scriptUrlParamInputs),
      ...Object.keys(htmlUrlParamInputs),
      ...Object.keys(htmlSlugParamInput),
    ],
    true,
  );

  return {
    ...data,
    // Pass any custom attributes to HTML content
    html: data.html
      ? createHtml(
          data.html.element,
          data.html.attributes,
          htmlAttrInputs,
          htmlUrlParamInputs,
          htmlSlugParamInput,
        )
      : undefined,
    // Pass any required query params with user values for relevant scripts
    scripts: data.scripts
      ? data.scripts.map((script) => {
          return isExternalScript(script)
            ? {
                ...script,
                url: formatUrl(script.url, allScriptParams, scriptUrlParamInputs, undefined, script.optionalParams), // add this
              }
            : {
                ...script,
                code: formatCode(script.code, scriptUrlParamInputs, script.optionalParams), // add this
              };
        })
      : undefined,
  };
}

Copy link
Collaborator

@flashdesignory flashdesignory left a comment

Choose a reason for hiding this comment

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

🚀

@flashdesignory flashdesignory merged commit 748a45b into GoogleChromeLabs:main Jul 27, 2024
2 checks passed
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.

Add optional parameters in gtm and ga json
4 participants