Resolve macros before encoding for analytics variables #26271
Conversation
micajuine-ho
force-pushed
the
expand_before_encode
branch
from
9bebe1e
to
9ef788b
Compare
micajuine-ho
force-pushed
the
expand_before_encode
branch
from
9ef788b
to
b95555c
Compare
I caused these conflicts :'-) |
micajuine-ho
force-pushed
the
expand_before_encode
branch
2 times, most recently
from
2811575
to
77e9f79
Compare
micajuine-ho
force-pushed
the
expand_before_encode
branch
from
77e9f79
to
d7ab181
Compare
|
cc @estherkim for |
|
cc @jridgewell for |
src/string.js
Outdated
| const replacementPromise = | ||
| typeof replacer === 'function' | ||
| ? replacer.apply(null, arguments) | ||
| : replacer; |
There was a problem hiding this comment.
The type says this is always a function. How do we get to this else branch?
There was a problem hiding this comment.
I think that this is just a bad type on my part, was trying to keep it consistent with string.replace where the replacer can be a {Function|string}. e.g. test-string.js L170 in this PR.
There was a problem hiding this comment.
Then we need to check it earlier. We can't support the string replacer here, since we've already passed a function to string.replace(). But if you check before calling string replace, you can just Promise.resolve(string.replace(regex, replacment)).
There was a problem hiding this comment.
Sorry I don't think I understand the difference. We are only really using native string.replace to get the matches & offset? Can you give an example that breaks it?
There was a problem hiding this comment.
't123'.replace(/t(\d+)/g, '$1'). See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replace#Specifying_a_string_as_a_parameter, the string replacement value is much more complicated then most realize. Let's either forbid a string, or support it properly.
There was a problem hiding this comment.
ah thanks, you are totally right. Moving the check sgtm.
There was a problem hiding this comment.
also @micajuine-ho can we add a test case or two using these "special replacement patterns" :)
src/string.js
Outdated
| const stringBuilder = []; | ||
| let lastIndex = 0; | ||
| if (typeof replacer === 'string') { | ||
| stringBuilder.push(Promise.resolve(str.replace(regex, replacer))); |
There was a problem hiding this comment.
nit: no need to use stringBuilder here, I think the idea was just return Promise.resolve(str.replace(...))) @jridgewell to confirm.
There was a problem hiding this comment.
Yeah, that makes sense 👍
| return expect(result).to.eventually.equal('the quick red fox'); | ||
| }); | ||
|
|
||
| it('should use replacer with special pattern', async () => { |
micajuine-ho
force-pushed
the
expand_before_encode
branch
from
207c670
to
3dc0526
Compare
| * @param {Function|string} replacer | ||
| * @return {!Promise<string>} | ||
| */ | ||
| export function asyncStringReplace(str, regex, replacer) { |
micajuine-ho
force-pushed
the
expand_before_encode
branch
from
d7fcb74
to
b6c1da3
Compare
…l into expand_before_encode
micajuine-ho
force-pushed
the
expand_before_encode
branch
from
b6c1da3
to
279fb66
Compare
micajuine-ho
requested review from
zhouyx and
calebcordry
and removed request for
zhouyx
Clean PR copy + merge fixes of #22379. Closes #21751.
In amp-analytics, as we recursively call
expandTemplate(), we want to resolve macros (throughurl-replacement-service) as we encounter them to avoid premature encoding (eg:QUERY_PARAM(foo, (QUERY_PARAM(bar, default))should returndefaultinstead of encoding the second QUERY_PARAM).There are caveats to this approach, so documentation will be added in this PR to address it. See test cases in
test-variables.js.