Resolve macros in VariableService. by calebcordry · Pull Request #22379 · ampproject/amphtml

calebcordry · 2019-05-19T18:40:30Z

Changes VariableService.expandTemplate to resolve macros through the UrlReplacementService as it encounters them. This avoids the problem of the variableService encoding certain macros before they are resolved. $, ( and , would be encoded in nested macros etc.

One note: I had to pass the element along everywhere this is called to check if amp-analytics is sandboxed. I am open to other suggestions on how this could be accomplished.

lannka · 2019-06-13T15:40:22Z

Is this PR blocked by anything? Or is it ready for review?

calebcordry · 2019-06-13T17:04:22Z

@lannka I keep forgetting about this PR, but it is ready for review.

zhouyx

@calebcordry Can we sync on the fix offline sometime? I want to understand what approaches has been discussed before.

From what I found, you're trying to expand the macros in variables before replacing them. Which avoids the encoding issue. However I have two concerns. 1. expandTemplate and expandUrlAsync used to be two different expansion. With this change we start to mix them together. 2. Instead of trying to resolve macro at event trigger time, we may resolve macros too early. TIMESTAMP for example.

zhouyx · 2019-06-18T20:44:09Z

extensions/amp-analytics/0.1/variables.js

    this.linkerReader_ = linkerReaderServiceFor(this.ampdoc_.win);

+    /** @const @private {!../../../src/service/url-replacements-impl.UrlReplacements} */
+    this.urlReplacementService_ = Services.urlReplacementsForDoc(


We can't do this. Instead we'll have to call Services.urlReplacementsForDoc(element) for every element we want to expand. Because FIE doesn't have its own ampdoc, but it has its own urlReplacementService.

Moved to get urlReplacementService from the element inside the expandTemplate call.

calebcordry · 2019-06-18T22:17:31Z

Sounds good, let's plan on syncing sometime Thursday.

zhouyx · 2019-06-18T22:20:45Z

sgtm

calebcordry · 2019-07-09T21:18:39Z

@zhouyx @lannka I think this is now in a good updated state for review.

zhouyx · 2019-07-10T19:10:25Z

extensions/amp-analytics/0.1/resource-timing.js

 * @return {!Promise<string>}
 */
-function serializeResourceTiming(ampdoc, resourceTimingSpec) {
+function serializeResourceTiming(ampdoc, element, resourceTimingSpec) {


nit: can we get rid of ampdoc here?

I think we need it to get the win? Let me know if I am missing something.

Still not sure about this one.

amphtml/src/custom-element.js

Line 337 in b0bf86a

getAmpDoc() {

Oh I see, thanks, done.

zhouyx · 2019-07-10T19:12:34Z

extensions/amp-analytics/0.1/test/test-amp-analytics.js

        'p:nth-child(2)',
      ].map(selectorExpansionTest);
-
-      it('does not expands selector with platform variable', () => {


Any way to preserve the existing behavior?

Is there a reason we don't want to use the platform macros here? We could add something like a noPlatform flag for certain callers if you like. This would come with the cost that these callers could not use nested.

This behavior has changed, let me know if you think we need to support not expanding macros in certain cases.

I guess it's fine. Please add a note to the code. Thanks

Added a note to expandTemplate

extensions/amp-analytics/0.1/variables.js

mrjoro · 2019-08-08T19:27:02Z

Is this PR still active?

calebcordry · 2019-08-08T19:29:48Z

Yep working on it now.

calebcordry · 2019-08-08T20:19:37Z

@zhouyx I think this is finally ready for another round of review

zhouyx · 2019-08-08T21:41:41Z

extensions/amp-analytics/0.1/resource-timing.js

 * @return {!Promise<string>}
 */
-function serializeResourceTiming(ampdoc, resourceTimingSpec) {
+function serializeResourceTiming(ampdoc, element, resourceTimingSpec) {


amphtml/src/custom-element.js

Line 337 in b0bf86a

getAmpDoc() {

zhouyx · 2019-08-08T21:45:54Z

extensions/amp-analytics/0.1/test/test-variables.js

-    });
+      })
+        .then(() =>
+          // TODO: fix this, should be 'AAA(BBB(1,2))'


nit: remove since we are not going to fix it.

zhouyx · 2019-08-08T21:45:58Z

extensions/amp-analytics/0.1/test/test-variables.js

+          // valid macros when they are seen in `Variables#expandTemplate`.
+          check('${foo}', 'AAA(BBB(1%2C2))', {
+            'foo': 'AAA(BBB(1,2))',
+          })


zhouyx · 2019-08-08T21:46:16Z

extensions/amp-analytics/0.1/test/test-variables.js

+          })
+        )
+        .then(() =>
+          // TODO: fix this, should be 'AAA(1,2)%26BBB(3,4)%26CCC(5,6)%26DDD(7,8)


nit: remove TODO

zhouyx · 2019-08-08T22:04:36Z

extensions/amp-analytics/0.1/variables.js

-  expandTemplateSync(template, options) {
-    return template.replace(/\${([^}]*)}/g, (match, key) => {
+  expandTemplate(template, options, element) {
+    return this.asyncStringReplace_(template, /\${([^}]*)}/g, (match, key) => {


You may have explained this before. I'm not an expert on regex.
${abc${def}} would break right? If so can we add a test to cover that. Thanks

That is correct. Added test.

zhouyx · 2019-08-08T22:14:08Z

extensions/amp-analytics/0.1/variables.js

+   * @param {Function} replacer
+   * @return {!Promise<string>}
+   */
+  asyncStringReplace_(str, regex, replacer) {


If I understand the code correctly. What we want here to an array of promise that waits for each variable to resolve. Personally I think this helper method makes the code more difficult to understand.
Can we inline the code as previous and do
const promises = []
str.replace(regex, () => {
// some operation
promises.push(promise)
})
return Promise.all(promises)

So that we can get rid of the stringBuilder here. Thanks

Talked offline. Moved asyncStringReplace to a separate function in string.js

zhouyx · 2019-08-08T22:15:52Z

extensions/amp-analytics/0.1/test/test-amp-analytics.js

        'p:nth-child(2)',
      ].map(selectorExpansionTest);
-
-      it('does not expands selector with platform variable', () => {


I guess it's fine. Please add a note to the code. Thanks

zhouyx · 2019-08-08T22:17:51Z

extensions/amp-analytics/0.1/test/test-variables.js


    it('expands nested vars (encode once)', () => {
-      check('${a}', 'https%3A%2F%2Fwww.google.com%2Fa%3Fb%3D1%26c%3D2', vars);
+      return check(


Are there test coverage for expanding the nested macro? Please help link to the test. Thanks.
Also given there are a bunch of special cases, it would be great to add a manual test file which helps demonstrate all the expected behaviors. Thanks

Added a manual file, let me know if there is anything else that you can think of. There is a test here

amphtml/extensions/amp-analytics/0.1/test/test-variables.js

Lines 148 to 153 in d165120

.then(() =>

check('${nested}', 'default', {

'nested': '${deeper}',

'deeper': '$IF(true, QUERY_PARAM(foo, default), never)',

})

));

that shows the new behavior, let me know if have idea for more.

calebcordry · 2019-08-16T20:10:41Z

@zhouyx this should be ready for another round of review whenever you have time. Thanks!

zhouyx · 2019-08-17T00:00:29Z

extensions/amp-analytics/0.1/amp-analytics.js

  expandTemplateWithUrlParams_(spec, expansionOptions) {
    return this.variableService_
-      .expandTemplate(spec, expansionOptions)
+      .expandTemplate(spec, expansionOptions, this.element)


Since we pass the binding info to expandUrlAsync, do we want to pass the same to expandTemplate?

I think this only matters for RESOURCE_TIMING as the way expandTemplate is now written it will try to get the bindings from the element if not passed in. If you prefer to explicitly pass it the bindings everywhere, I am happy to do so.

zhouyx · 2019-08-17T00:01:10Z

extensions/amp-analytics/0.1/linker-manager.js

    const bindings = this.variableService_.getMacros(this.element_);
    return this.variableService_
-      .expandTemplate(template, expansionOptions)
+      .expandTemplate(template, expansionOptions, this.element_)


zhouyx · 2019-08-17T00:01:38Z

extensions/amp-analytics/0.1/requests.js

      this.requestOriginPromise_ = this.variableService_
        // expand variables in request origin
-        .expandTemplate(this.requestOrigin_, requestOriginExpansionOpt)
+        .expandTemplate(


This one should definitely have the bindings, as it will include the RESOURCE_TIMING macro. Done.

zhouyx · 2019-08-17T00:01:48Z

extensions/amp-analytics/0.1/requests.js


  const basePromise = variableService
-    .expandTemplate(msg, expansionOption)
+    .expandTemplate(msg, expansionOption, element)


zhouyx · 2019-08-17T00:03:40Z

extensions/amp-analytics/0.1/test/test-variables.js


    it('expands nested vars (encode once)', () => {
-      check('${a}', 'https%3A%2F%2Fwww.google.com%2Fa%3Fb%3D1%26c%3D2', vars);
+      return check(


zhouyx · 2019-08-17T00:04:48Z

src/string.js

+ * @param {Function} replacer
+ * @return {!Promise<string>}
+ */
+export function asyncStringReplace(str, regex, replacer) {


Please add unit test for this function

Actually hit a bug here with variable number of capture groups. Didn't think of this when we moved to make it generic. Working on fix.

OK fixed :) should work with any number of capture groups.

zhouyx · 2019-08-17T00:09:54Z

test/manual/amp-analytics/analytics-nested.html

+    You can use the ${} format in conjuction with nested macros, as long as there are no nested ${}.
+    See combo below.
+   -->
+  <amp-analytics>


Thanks for the manual test file

It was a good idea 🙂. You are welcome.

calebcordry · 2019-08-22T18:32:45Z

@zhouyx I think this is ready for another round whenever you have time. Thanks!

lannka · 2020-01-27T21:24:11Z

is this still needed? or did @micajuine-ho take over?

lannka · 2020-01-27T21:25:06Z

saw #26271

googlebot added the cla: yes label May 19, 2019

calebcordry changed the title R Resolve macros in VariableService. May 19, 2019

calebcordry requested review from lannka and zhouyx May 21, 2019 18:29

calebcordry marked this pull request as ready for review May 21, 2019 18:30

zhouyx suggested changes Jun 18, 2019

View reviewed changes

calebcordry mentioned this pull request Jul 3, 2019

getNameArgs in variables.js doesn't understand nested macros #21751

Closed

zhouyx suggested changes Jul 11, 2019

View reviewed changes

calebcordry added 5 commits August 8, 2019 12:46

resolve macros in vars

f2decd8

add test

07b0564

update for request origin

c9a8299

whitelist expandStringAsync

6464458

pass element to getMacros

7c78bbc

remove commented out

8c8baf8

amp-bundle-size bot requested a review from dreamofabear August 8, 2019 21:26

zhouyx suggested changes Aug 8, 2019

View reviewed changes

calebcordry added 6 commits August 8, 2019 17:01

easy comments

4c5fe97

move asyncReplace

9916eb0

remove ampdoc from serializeResourceTiming

8e92011

more comments

40bd5b6

pass in whitelist and bindings

13ab0c9

fix resourceTiming test

797c872

calebcordry added 2 commits August 9, 2019 16:42

manual doc file

d165120

jsdoc

e760e34

zhouyx reviewed Aug 17, 2019

View reviewed changes

dreamofabear removed their request for review August 19, 2019 19:44

calebcordry added 3 commits August 19, 2019 15:10

pass binding for requests

3ca8891

asyncStringReplace test

2ee0787

fix async replace

7179c9c

calebcordry mentioned this pull request Sep 18, 2019

✨ Add Contentsquare analytics vendor #23813

Closed

zhouyx approved these changes Sep 18, 2019

View reviewed changes

lannka approved these changes Nov 23, 2019

View reviewed changes

zhouyx mentioned this pull request Jan 2, 2020

Avoid encoding data-amp-addparams #26172

Merged

micajuine-ho mentioned this pull request Jan 9, 2020

Resolve macros before encoding for analytics variables #26271

Merged

lannka closed this Jan 27, 2020

	.then(() =>
	check('${nested}', 'default', {
	'nested': '${deeper}',
	'deeper': '$IF(true, QUERY_PARAM(foo, default), never)',
	})
	));

Conversation

calebcordry commented May 19, 2019

Uh oh!

lannka commented Jun 13, 2019

Uh oh!

calebcordry commented Jun 13, 2019

Uh oh!

zhouyx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calebcordry commented Jun 18, 2019

Uh oh!

zhouyx commented Jun 18, 2019

Uh oh!

calebcordry commented Jul 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mrjoro commented Aug 8, 2019

Uh oh!

calebcordry commented Aug 8, 2019

Uh oh!

calebcordry commented Aug 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calebcordry commented Aug 16, 2019