Changes between Initial Version and Version 1 of Ticket #25985, comment 25


Ignore:
Timestamp:
Jul 2, 2018, 11:49:59 AM (17 months ago)
Author:
twim
Comment:

Legend:

Unmodified
Added
Removed
Modified
  • Ticket #25985, comment 25

    initial v1  
    66> (On further reflection, what I was trying to do wouldn't have worked anyway, because the AMP client code would have sent a URL path of `/c/s/.../amp/client/...`, not `/amp/client/...`, but I could have worked around that for testing.) It looks like the cause is [https://github.com/nogoegst/amper/blob/6dbbc42eb44c1f389591c83a268524d5eceed1d1/client.go#L55 here in amper/client.go], with a hardcoded `"https"`. Looking at that, I also noticed a [https://github.com/nogoegst/amper/blob/6dbbc42eb44c1f389591c83a268524d5eceed1d1/client.go#L23 CDNDomain = "cdn.ampproject.org"]. I have to say, it skeeves me a bit to see hardcoded constants like that in a library; it seems like the kind of thing that belongs in a configuration file.
    77
    8 Thanks to your suggestion I made some constants configurable. One can now specify Client.Scheme and Client.CDNDomain.
     8Thanks to your suggestion I made some constants configurable. One can now specify `Client.Scheme` and `Client.CDNDomain`.
    99As you may guess, it was intended to work with the only AMP cache around thus the hardcoded values.
    1010
     
    1616>  * The `<style>body {opacity: 0}</style><noscript><style>body {opacity: 1}</style></noscript>` doesn't match the documented [https://www.ampproject.org/docs/reference/spec/amp-boilerplate.html AMP boilerplate code], which is much longer. Is there a reason?
    1717This is knowm as "old boilerlate" (https://github.com/ampproject/amphtml/commit/0a056ca50ac8cb9ba8e5a6489baeecb5ed958556#diff-17672f7dfc8b0583c360b45234dbf59a). The reason is that it's much shorter than the new one. I've picked this as a tradeoff to save bandwidth as the old boilerplate is still accepted by AMP CDN.
    18 Now this is also configurable: set Server.UseOldAMPBoilterplate to true to use shorter boilerplate, or get completely valid 'modern' AMP page by default.
     18Now this is also configurable: set `Server.UseOldAMPBoilterplate` to `true` to use shorter boilerplate, or get completely valid 'modern' AMP page by default.
    1919
    2020>  * `<link rel="canonical" href="https://ampproject.org" />`: it would be better not to refer to any third parties. The AMP guide says this can be a relative URL, so I would prefer one of these if they work: