Add url REPLACE macro #13130
Add url REPLACE macro #13130
Conversation
| @@ -193,6 +193,14 @@ describe('amp-analytics.VariableService', function() { | |||
| 'S7Uc5ZmODduHWdplzrZ7Jsnqx') | |||
| ); | |||
| }); | |||
|
|
|||
| it('replaces', () => { | |||
| return check('REPLACE(this-is-a-test, `-`, )', 'thisisatest'); | |||
There was a problem hiding this comment.
i thought this should gives this is a test (with spaces)
There was a problem hiding this comment.
we are currently stripping spaces so people can do things like DEFAULT(one, two) vs only allowing DEFAULT(one,two). We could change this but I think allowing it is better? It does make it tricky to pass in a space though
There was a problem hiding this comment.
Isn't that both tests are testing replace with no third arg? here newSubStr = undefined?
There was a problem hiding this comment.
no, newSubStr === '' in the first. I will add a few more tests to make this more clear
There was a problem hiding this comment.
so how do i get this is a test? Is it supported?
There was a problem hiding this comment.
I was spending some time thinking about this last night. Right now it is not supported without using a different macro that would return a space. We could support something like REPLACE(this-is-a-test, `-`, ` `) If we do that should we make the back ticks on the third parameter required? Also no matter what the spaces will be encoded so it would return this%20is%20a%20test
There was a problem hiding this comment.
We are now respecting whitespaces inside all back ticks. More tests were written to cover this behavior.
| @@ -98,6 +98,17 @@ function defaultMacro(value, defaultValue) { | |||
| return value; | |||
| } | |||
|
|
|||
| function replaceFilter(string, matchPattern, newSubStr) { | |||
| @@ -193,6 +193,14 @@ describe('amp-analytics.VariableService', function() { | |||
| 'S7Uc5ZmODduHWdplzrZ7Jsnqx') | |||
| ); | |||
| }); | |||
|
|
|||
| it('replaces', () => { | |||
| return check('REPLACE(this-is-a-test, `-`, )', 'thisisatest'); | |||
There was a problem hiding this comment.
Isn't that both tests are testing replace with no third arg? here newSubStr = undefined?
| if (!ignoringChars) { | ||
| ignoringChars = true; | ||
| nextArgShouldBeRaw = true; | ||
| builder = ''; |
There was a problem hiding this comment.
user().assert(builder === '') first. This can hide bugs such as
build`test`
missing the build
There was a problem hiding this comment.
this is a good idea. Will make a separate PR since this is now merged
|
|
||
| const evaluateNextLevel = () => { | ||
| let builder = ''; | ||
| const results = []; | ||
|
|
||
| while (urlIndex < url.length && matchIndex <= matches.length) { | ||
|
|
||
| if (match && urlIndex === match.start) { |
There was a problem hiding this comment.
Why can we get a match in the middle of a literal string?
There was a problem hiding this comment.
If I understand your question correctly, this is to support things like example.com?cid=CLIENT_ID(__ga)
There was a problem hiding this comment.
No, I mean why would CLIENT_ID match inside of
cid=`CLIENT_ID(__GA)`
the tick marks a literal string ("don't parse me" section).
There was a problem hiding this comment.
the way it was designed is so that the ticks are only respected inside a macro call (inside parenthesis). We are currently reserving keywords like CLIENT_ID to be replaced anywhere in the input string. We could potentially change the parser to look for this if you think its a valid use case?
* add-replace-filter * Refactor space handling * Toggle off next arg * Do what test describes
* add-replace-filter * Refactor space handling * Toggle off next arg * Do what test describes
Follow up to #12682. All the new macros should now be available behind the
url-replacement-v2flag.This PR adds the
REPLACEmacro that can be used in url expansion. It also adds the ``` character that tells the new parser to ignore the special characters that may remain inside.Closes #2198