Migrate to AMP Optimizer 2.0 #235
Conversation
* replace amphtml-autoscript with optimizer built-in functionality * change template tags from <% to [% to avoid conflicts with the html parsers * move AMP Optimizer invocation to built-time instead of runtime * let AMP Optimizer inject AMP mandatory tags
sebastianbenz
requested review from
demianrenzulli,
morsssss and
antoinebisch
|
Thanks for doing this @sebastianbenz! The code lgtm. I'm especially glad to see you were able to get rid of the part where we override I'm wondering about the need to replace template delimiters. Why is this the case? I'm sorry I haven't had a chance yet to pull down the code and just try it out to make sure it works for me as well as it does for you. Soon! |
|
Ok. I tried it out. It works! I still have the question: why do we need to use different delimiters? Also, I did find one difference: with the new autoscript includer, one extraneous script gets included on product details page:
|
|
Thanks for checking!
The old delimiters were using
Interesting! Will investigate! |
|
You mean, they threw off the autoscriptinclusion parser that looks for AMP components in HTML tags? If so, I'd suggest changing the autoscriptinclusion parser, if that's possible. Because otherwise you risk breaking any site that uses the relatively common |
AMP Optimizer requires an HTML parser, so that's a difficult request. Running AMP Optimizer on templates is very clever, but requires precaution... |
|
Hi folks, Sorry for jumping late into this. Here are my 2 cents: I think @morsssss is right in that angle brackets might be a common way of writing delimiters in many templates engines. In the case of Mustache, the choice we made Based on previous comments, it seems like the options are:
Since none of the previous options are good, what we can do is:
Not sure if I'm missing something! These are just my thoughts. |
|
I'm torn here. We want to use AMP Optimizer. But I'm also concerned that we need to modify a common templating pattern (even if our templating scheme, as @sebastianbenz points out, is a little unusual). I'd defer to what others think. @pbakaus , @patrickkettner, @alankent, @fstanis , @antoinebisch , would love to get your opinion here! |
|
Do I have an opinion? Usually!!! ;-) I don't really care using What worries me more is running the optimizer over the template rather than the HTML output by the template. It's kinda cute, kinda dangerous. Who knows when something else will break in the future. The optimizer is not being given valid HTML content - it is being given a text templates that output HTML. E.g. class names or attributes may be substituted in by the templating language. The optimizer won't see them, and that feels risky to me. Even if it works today, it could break at any time in the future. I was not sure why the optimizer could not be run over the final generated page. To me that makes the most sense in terms of ultimately reliability and safety. You should feed HTML markup (not templating language markup) into the optimizer. That's my PoV anyway. |
|
@alankent , it is a little unusual. @sebastianbenz pointed out the same thing! The advantage here is that we can run it at build time for pages that are dynamically generated. But, yes, there are disadvantages. |
Well said Alan. The more I think about this, the more I think that this approach is not a good idea. Validating templates won't give you 100% confidence that the final pages are valid AMP. We definitely shouldn't be recommending this approach as it also seriously limits the expressiveness of your templates. For example, this common use case won't work: |
|
Well, we did use templating like this:
Anyway, @alankent , the current site does in fact run the optimizer in the server. As far as I can tell it does run the optimizer over the final rendered pages. But now the optimizer has new functionality - it replaces the amphtml-autoscript module used in the current site. It automatically adds the scripts required by AMP components. So @sebastianbenz 's fork uses that functionality and thus it runs the optimizer at build time. This is nice for a few reasons... among which, there's no longer a need to cache optimized pages in the server. I'm not really here to defend the current build system, which has a couple of things we'd want to change if the site got more complex. So far, @alankent doesn't think so. If no one else who wasn't involved in the project thinks this is a problem, I'm happy to approve this PR as is! I wish I had time to go back and rework this build process, but at this point I think it's best to get this out there and move on to samples that use |
|
Sorry for coming back to this thread a bit late.
I don't have a strong opinion on the ability to use <% or not. In the end,
mustache is flexible enough to allow other characters to be used so we can
work around this limitation. It will just take us a bit of time and testing
to change our code to a different pattern.
+1 to running optimizer on top of the rendered pages though. I recall this
was creating another issue in the past with some HTML tags not being
recognised as valid because of the server side logic, although they would
become valid once rendered. I suppose one of the difficulties here is that
some of our server side variables are swapped by URL parameters so it could
be problematic to generate the code only from the back-end, prior to
running optimizer, in that context?
<https://www.google.com/doodles/44th-anniversary-of-the-birth-of-hip-hop>
* • **Antoine Bisch*
* • *Mobile Solutions Consultant
* • *1-13 St Giles High Street, WC2H 8AG, London
• [email protected]
<[email protected]>
This email may be confidential and privileged. If you received this
communication by mistake, please don't forward it to anyone else, please
erase all copies and attachments, and please let me know that it has gone
to the wrong person.
The above terms reflect a potential business arrangement, are provided
solely as a basis for further discussion, and are not intended to be and do
not constitute a legally binding obligation. No legally binding obligations
will be created, implied, or inferred until an agreement in final form is
executed in writing by all parties involved.
…On Thu, 30 Jan 2020 at 23:49, Ben Morss ***@***.***> wrote:
Well, we did use templating like this:
<amp-img option="<%ColorName%>" src="<%& Photo%>">
Anyway, @alankent <https://github.com/alankent> , the current site does
in fact run the optimizer in the server. As far as I can tell it does run
the optimizer over the final rendered pages. But now the optimizer has new
functionality - it replaces the amphtml-autoscript module used in the
current site. It automatically adds the scripts required by AMP components.
So @sebastianbenz <https://github.com/sebastianbenz> 's fork uses that
functionality and thus it runs the optimizer at build time. This is nice
for a few reasons... among which, there's no longer a need to cache
optimized pages in the server.
I'm not really here to defend the current build system, which has a couple
of things we'd want to change if the site got more complex. fileinclude
was already starting to strain, for example. My question is, is it a
problem to disallow the <% and %> templates?
So far, @alankent <https://github.com/alankent> doesn't think so. If no
one else who wasn't involved in the project thinks this is a problem, I'm
happy to approve this PR as is!
I wish I had time to go back and rework this build process, but at this
point I think it's best to get this out there and move on to samples that
use <amp-script>! If someone else feels strongly that we shouldn't, let
me know. I remember some other prominent AMP projects that suffered from
build processes that didn't scale... ampstart, anyone? Not saying this is
ideal, just saying that the templating process isn't the main idea here.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#235?email_source=notifications&email_token=AJC5E5WBSB54UDORNKGMOMDRANRRVA5CNFSM4KKV6ED2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKM7PNY#issuecomment-580515767>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJC5E5WI5PR2K2HBZTOI4IDRANRRVANCNFSM4KKV6EDQ>
.
|
|
@morsssss |
|
@sebastianbenz, yes, exactly. We never faced this issue, but, of course, others who adopt our approach would face it eventually. To really do this right, I fear a developer would have to run the optimizer during the build process and once again in the server. This worries me. On the other hand, our current scheme worries me more. So since no one else is really concerned about the loss of the angle-bracket template delimiters, I'm going to merge this PR. I agree this is the best compromise for now. And if we ever revisit this project, we'll see if we can create a more sustainable build process. I finally tried running the optimizer on the templates, and I was surprised to find that the parser didn't barf. It simply parsed the delimiters as HTML tags the best way that it could, and ultimately the optimizer transformed things like this:
to things like this:
which leads to unexpected - and hard to debug - results in the browser. Even if AMP CAMP's templating and build is not ideal, I don't think this is a desirable outcome. The optimizer has to just work in predictable ways, no matter what silly things users might do! So I've made an issue to tackle this, either by issuing a warning in such cases, or by preprocessing/postprocessing HTML to allow this approach. Also, as time permits, would love to see this optimizer issue dealt with. The real world is full of weirdos like us who use your product in ways that you would not have expected 😎 Thanks to everyone for this excellent discussion! |
parsers