SSR template rendering #3 - re-format requests by GoTcWang · Pull Request #18189 · ampproject/amphtml

GoTcWang · 2018-09-19T17:43:34Z

Reformat the AmpComponent passed to viewer, to make sure ssr-template-helper:

returns successTemplate or errorTemplate as null if it doesn't exist, rather than now always returns both templates but payload might be null
puts additionalAttr which correctly only has ampListAttributes under AmpComponent where makes more sense
uses hasAttribute() rather than getAttribute() for singleItem which is a boolean to be consistent with normal amp-list and also prevent bugs where different browsers have inconsistent behaviors on whether to return an empty string when the attribute doesn't exist
provides AMP CORS query parameters for amp-form which always includes credentials
returns innerHTML for all templates node to get ride of outer tag
This also fixed a bug where if some template doesn't not exist in amp-form, xmls_.serializeToString will throw an error saying that:

'''
Form submission failed: TypeError: Failed to execute 'serializeToString' on 'XMLSerializer': parameter 1 is not of type 'Node'.
'''

TESTED=amp-list with items=".", singleItem=true and amp-form with no templates
I will test more later but want to check in this first to make sure our first bunch of AMP4EMAIL(s) work.

googlebot · 2018-09-19T17:43:38Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

GoTcWang · 2018-09-19T17:45:09Z

@zhangsu @choumx

GoTcWang · 2018-09-19T22:22:49Z

Please hold, this doesn't work. I'll ping this thread again when it's ready.

Sorry Will!

GoTcWang · 2018-09-20T21:40:41Z

Ping @choumx and @zhangsu.
Mind taking a look? This should be good enough for at least the first round.

src/ssr-template-helper.js

extensions/amp-list/0.1/amp-list.js

zhangsu · 2018-09-21T18:06:51Z

extensions/amp-form/0.1/amp-form.js

              dev().assertString(this.xhrAction_), this.method_);
-          setupInit(request.fetchOpt);
-          setupAMPCors(this.win_, request.xhrUrl, request.fetchOpt);
+          request.fetchOpt = setupInit(request.fetchOpt);


So the setupXxx function actually has a side effect (it mutates the passed-in argument), so maybe it should be left in the previous state where the return value is simply ignored... When I see a function that takes an input and returns an ouput, I often assume that it's side effect-free.

Hmmm problem is we have to use setupInit this way, because it doesn't mutate passed-in parameters.

https://github.com/ampproject/amphtml/blob/master/src/utils/xhr-utils.js#L250

I used others this way for consistency, but can definitely change them if that's better.

I looked at that code and it definitely looks like to me it's mutating the passed-in opt_init. It's just saving another init reference to opt_init and mutating through the init reference, but it's still mutating the same object in memory.

I mean, you didn't modify those setupXxx functions in this PR, and the existing code on master is not using the return values, so the existing code must be relying on these functions to modify the passed-in init in place.

Yea, the old code was just wrong. Thanks for fixing this.

Hmmm I wasn't so sure about the correctness previously 😟
I remember it doesn't work until I made it this way, but it could because there wan't setupInput before.

And actually there are using them like this way in the other reference (from amp-list):
https://github.com/ampproject/amphtml/blob/master/src/service/xhr-impl.js#L113

src/ssr-template-helper.js

zhangsu · 2018-09-21T18:56:39Z

extensions/amp-form/0.1/amp-form.js

              dev().assertString(this.xhrAction_), this.method_);
-          setupInit(request.fetchOpt);
-          setupAMPCors(this.win_, request.xhrUrl, request.fetchOpt);
+          request.fetchOpt = setupInit(request.fetchOpt);


I looked at that code and it definitely looks like to me it's mutating the passed-in opt_init. It's just saving another init reference to opt_init and mutating through the init reference, but it's still mutating the same object in memory.

I mean, you didn't modify those setupXxx functions in this PR, and the existing code on master is not using the return values, so the existing code must be relying on these functions to modify the passed-in init in place.

src/ssr-template-helper.js

zhangsu · 2018-09-21T19:17:33Z

src/ssr-template-helper.js

-      'successTemplate': {
+    const ampComponent = dict({'type': this.sourceComponent_});
+
+    const successTemplate = (opt_templates && opt_templates['successTemplate'])


If you pass in an actual template object to mustacheTemplate (which can probably be better-named, since currently there's mustacheTemplate and opt_templates which is kinda confusing), then you can just do:

const successTemplate = (opt_templates && opt_templates['successTemplate']) ? opt_templates['successTemplate'] : mustacheTemplate; if (successTemplate) { ... 'payload': successTemplate.innerHTML, ... }

Hmmm does it really work? mustacheTemplate (or ampListTemplate now) can also be null.

If mustacheTemplate (or ampListTemplate now) is null, it will fail the if (successTemplate) check, wouldn't it? I'm not sure if you need all those conditionals... seems redundant

I see what you mean, yes this works, done.

dreamofabear · 2018-09-21T19:14:13Z

extensions/amp-form/0.1/amp-form.js

              dev().assertString(this.xhrAction_), this.method_);
-          setupInit(request.fetchOpt);
-          setupAMPCors(this.win_, request.xhrUrl, request.fetchOpt);
+          request.fetchOpt = setupInit(request.fetchOpt);


Yea, the old code was just wrong. Thanks for fixing this.

dreamofabear · 2018-09-21T19:20:16Z

src/ssr-template-helper.js

-      },
-      'errorTemplate': {
+        'payload': successTemplate
+          ? successTemplate : mustacheTemplate,


Nit: successTemplate || mustacheTemplate

Can't find that code version anymore but seems not necessary anymore, we have a
if (ampFormSuccessTemplate || ampListTemplate) {
out of it.

src/ssr-template-helper.js

dreamofabear · 2018-09-21T19:23:22Z

src/ssr-template-helper.js

    if (additionalAttr) {
      Object.keys(opt_attributes).forEach(key => {
-        data[key] = opt_attributes[key];
+        ampComponent[key] = opt_attributes[key];


Not your fault but additionalAttr is a bit weird.

This should suffice:

if (opt_attributes) { Object.assign(ampComponent, opt_attributes); }

Done. This is nice!

dreamofabear · 2018-09-21T19:33:11Z

src/ssr-template-helper.js

-        // serializeToString for a string representation of the dom tree.
-        mustacheTemplate = this.xmls_.serializeToString(
-            this.templates_.findTemplate(element));
+        mustacheTemplate = template./*REVIEW*/innerHTML;


Eh, we can probably make this API clearer. /cc @alabiaga

fetchAndRenderTemplate(element, request, successTemplate, opt_errorTemplate = null, opt_attributes = {})

No action needed. :)

Or, I can make this
fetchAndRenderTemplate(element, request, ampListTemplate, ampFormTemplates = null, opt_attributes = {})
for now which might be slightly clearer(?)
And won't involve too much work.

It's ok, I'd rather not name the params by its callers.

zhangsu · 2018-09-21T20:51:15Z

Ideally we need some unit tests to prevent bugs like this in the future...

GoTcWang · 2018-09-21T21:07:09Z

Ideally we need some unit tests to prevent bugs like this in the future...

Agree.
However I might want to defer tests later and check this in first. Just so that we can enable SSR earlier.

ghost · 2018-09-24T18:25:48Z

This pull request introduces 1 alert when merging e7fcecb into 146cde4 - view on LGTM.com

new alerts:

1 for Useless conditional

Comment posted by LGTM.com

codecov-io · 2018-09-24T20:36:06Z

Codecov Report

Merging #18189 into master will decrease coverage by 62.7%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           master   #18189       +/-   ##
===========================================
- Coverage    85.7%   22.99%   -62.71%     
===========================================
  Files        1270     1270               
  Lines      121893   121897        +4     
===========================================
- Hits       104471    28036    -76435     
- Misses      17422    93861    +76439

Flag	Coverage Δ
#unit_tests	`22.99% <0%> (-62.71%)`	⬇️

Impacted Files	Coverage Δ
extensions/amp-list/0.1/amp-list.js	`0.97% <ø> (-79.13%)`	⬇️
extensions/amp-form/0.1/amp-form.js	`1.77% <0%> (-83.69%)`	⬇️
src/ssr-template-helper.js	`0% <0%> (-39.14%)`	⬇️
src/mode-object.js	`0% <0%> (-100%)`	⬇️
extensions/amp-analytics/0.1/scroll-manager.js	`0% <0%> (-100%)`	⬇️
...tensions/amp-ad-exit/0.1/filters/click-location.js	`0% <0%> (-100%)`	⬇️
extensions/amp-access/0.1/access-expr.js	`0% <0%> (-100%)`	⬇️
extensions/amp-subscriptions/0.1/url-builder.js	`0% <0%> (-100%)`	⬇️
extensions/amp-story/1.0/amp-story-base-layer.js	`0% <0%> (-100%)`	⬇️
extensions/amp-bind/0.1/bind-macro.js	`0% <0%> (-100%)`	⬇️
... and 861 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4272e32...226f5e5. Read the comment docs.

GoTcWang · 2018-09-25T15:15:27Z

check cla...

googlebot · 2018-09-25T15:26:15Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

googlebot · 2018-09-25T15:29:33Z

CLAs look good, thanks!

GoTcWang · 2018-09-25T16:28:11Z

@choumx
Finally all tests passed! Could you please help me merge this PR?

dreamofabear · 2018-09-25T16:42:15Z

Nice work TC!

* fix AmpComponent format * lint * fix * add more * add /*review*/ for .innerHTML * comments * comments round 2 * comments round 3 * comments round 4 * revert one more * FIX TYPE

…18189) * fix AmpComponent format * lint * fix * add more * add /*review*/ for .innerHTML * comments * comments round 2 * comments round 3 * comments round 4 * revert one more * FIX TYPE

googlebot added the cla: no label Sep 19, 2018

dreamofabear self-requested a review September 19, 2018 17:54

zhangsu reviewed Sep 21, 2018

View reviewed changes

dreamofabear approved these changes Sep 21, 2018

View reviewed changes

zhangsu approved these changes Sep 24, 2018

View reviewed changes

GoTcWang force-pushed the ssr-3 branch from c287ff7 to 226f5e5 Compare September 24, 2018 20:18

GoTcWang force-pushed the ssr-3 branch 4 times, most recently from bd7763a to 574a931 Compare September 25, 2018 15:05

GoTcWang force-pushed the ssr-3 branch from 574a931 to 41e16b9 Compare September 25, 2018 15:25

GoTcWang added 8 commits September 25, 2018 11:28

fix AmpComponent format

190c163

lint

d522ec9

fix

af74dd2

add more

78c74c8

add /*review*/ for .innerHTML

5c65eec

comments

f4b58d8

comments round 2

e991efc

comments round 3

233255c

GoTcWang added 3 commits September 25, 2018 11:28

comments round 4

19603a4

revert one more

3625bb1

FIX TYPE

c19ae87

GoTcWang force-pushed the ssr-3 branch from 41e16b9 to c19ae87 Compare September 25, 2018 15:29

googlebot added cla: yes and removed cla: no labels Sep 25, 2018

Merge branch 'master' into ssr-3

ac11599

dreamofabear merged commit 5e46cc4 into ampproject:master Sep 25, 2018

Conversation

GoTcWang commented Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Sep 19, 2018

Uh oh!

GoTcWang commented Sep 19, 2018

Uh oh!

GoTcWang commented Sep 19, 2018

Uh oh!

GoTcWang commented Sep 20, 2018

Uh oh!

Uh oh!

Uh oh!

zhangsu Sep 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

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!

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!

zhangsu commented Sep 21, 2018

Uh oh!

GoTcWang commented Sep 21, 2018

Uh oh!

ghost commented Sep 24, 2018

Uh oh!

codecov-io commented Sep 24, 2018

Codecov Report

Uh oh!

GoTcWang commented Sep 25, 2018

Uh oh!

googlebot commented Sep 25, 2018

Uh oh!

googlebot commented Sep 25, 2018

Uh oh!

GoTcWang commented Sep 25, 2018

Uh oh!

dreamofabear commented Sep 25, 2018

Uh oh!

Reviewers

Assignees

GoTcWang commented Sep 19, 2018 •

edited

Loading

zhangsu Sep 21, 2018 •

edited

Loading