-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
馃悰Only expand strings in arrays #26324
Conversation
@@ -281,7 +281,10 @@ export class VariableService { | |||
} else if (isArray(value)) { | |||
// Treat each value as a template and expand | |||
for (let i = 0; i < value.length; i++) { | |||
value[i] = this.expandValue_(value[i], options); | |||
value[i] = | |||
typeof value[i] == 'string' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it ever be the case where value[i]
is an object or an array? Or anything that's not a number or a boolean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zhouyx We discussed in #26157, how arrays inside arrays would not be supported (unless they it is requested). I think the same approach should apply for objects.
Currently, our documentation does not say anything about objects, (e.g. they will just get url-encoded), and after testing, I can confirm that it does not break on objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right we don't want to support expanding nested array. Just to confirm that a nested array won't break the usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed. It does not break the usage. Should I add a test example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nah, should be good : )
Follow up to #26157