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

Analytics array variable substitutions #26157

Merged
merged 5 commits into from
Jan 9, 2020

Conversation

micajuine-ho
Copy link
Contributor

@micajuine-ho micajuine-ho commented Dec 26, 2019

For #21224

Variable substitutions can now be handled in arrays:

vars: {
  foo: bar,
  array: [${foo},'hello']
}

${array} -> 'bar,hello'

This will not work for nested arrays (see test case). Should we allow/disallow nested arrays? Or leave as is?

@lannka
Copy link
Contributor

lannka commented Dec 27, 2019

thanks for fixing this!

@micajuine-ho
Copy link
Contributor Author

@lannka Should we worry about nested array expansion?

@zhouyx
Copy link
Contributor

zhouyx commented Jan 2, 2020

LGTM Two questions

  1. Does this work with extraUrlParams as well?
  2. If we support nested objects within the extraUrlParams, like this
{
  'a': {
    'a1': 'b1', 
  },
  'b': 'b'
}

are we also supporting nested arrays like

{
  'a': {
    'a1': ['b1', 'b2'...], 
  },
  'b': 'b'
}

Thanks.

@micajuine-ho
Copy link
Contributor Author

@zhouyx

  1. Does this work with extraUrlParams as well?

extraUrlParams already handled arrayt

  1. If we support nested objects within the extraUrlParams, ... are we also supporting nested arrays...

Do you mean using nested arrays in extraUrlParams with useBody? If so, then yes. The nested array is sent within the POST request body.

Also just to clarify, expanding extraUrlParams does not work:

"extraUrlParams": {
  "a":  ["${b}", "bar"],
  "b": "foo"
}
// Will generate a url request with: a=&a=bar&b=foo

@zhouyx
Copy link
Contributor

zhouyx commented Jan 3, 2020

That should be fine. What about nested array within the array. For example

{
  'a': {
    'a1': [
     'b1', 
     ['c1', 'c2']
   ], 
  },
  'b': 'b'
}

Or it doesn't matter given the way we handle extraUrlParams in the request body?

@micajuine-ho
Copy link
Contributor Author

micajuine-ho commented Jan 3, 2020

Or it doesn't matter given the way we handle extraUrlParams in the request body?

Yes, it handle nested arrays in the request body.

@zhouyx
Copy link
Contributor

zhouyx commented Jan 3, 2020

Sounds good, thanks for confirming

@micajuine-ho
Copy link
Contributor Author

@zhouyx What I am worried about is arrays expanding arrays:

vars: {
  foo: 1,
  bar: 2,
  array: [${foo}, ${array2}]
  array2: [${bar}, 'hello']
}
// ${array} would ideally turn into 1,2,hello
// but instead turns into 1,2%2Chello (because array2 gets turned into a string and serialized)

Should we worry about this? To fix it would take a good amount of effort, I believe.

@micajuine-ho
Copy link
Contributor Author

Should we worry about this? To fix it would take a good amount of effort, I believe.

Discussed offline. We will leave as is and document that this is not the intended behavior.

If requested, we will fix.

@zhouyx
Copy link
Contributor

zhouyx commented Jan 8, 2020

SGTM. I'll let @lannka approve the PR since they've started the review : )

@micajuine-ho micajuine-ho merged commit 3c46dda into ampproject:master Jan 9, 2020
@micajuine-ho micajuine-ho deleted the array_vars branch January 9, 2020 17:26
value = this.expandValue_(value, options);
} else if (isArray(value)) {
// Treat each value as a template and expand
for (let i = 0; i < value.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The code breaks with an array like ["test", 123], because we are checking if the value type of string for items from the array.

This breaks use case like:

"request": "example.com?data=${data1}"    
"vars": {
      "abc": "${timestamp}.${requestCount}",
      "data1": ["ds", 123]
    },

Let's fix before next canary cut, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants