Improve support for self-hosted runtime #25026
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| <!doctype html> | ||
| <html ⚡> | ||
| <head> | ||
| <title>Self-hosted AMP test</title> | ||
| <meta charset="utf-8"> | ||
| <link rel="canonical" href="https://www/"> | ||
| <script async src="https://cdn.ampproject.org/v0.js"></script> | ||
| <script async custom-element="amp-geo" src="https://cdn.ampproject.org/v0/amp-geo-0.1.js"></script> | ||
| <script async custom-element="amp-form" src="https://cdn.ampproject.org/v0/amp-form-0.1.js"></script> | ||
| <meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1"> | ||
| <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript> | ||
| <style amp-custom> | ||
| body { | ||
| margin: 0; | ||
| padding: 8px; | ||
| font-family: sans-serif; | ||
| } | ||
| .geo-text, | ||
| body[class*="amp-geo"] .nogeo-text { | ||
| display: none; | ||
| } | ||
| .nogeo-text, | ||
| body[class*="amp-geo"] .geo-text { | ||
| display: block; | ||
| } | ||
| h3 { | ||
| margin-bottom: 0.5em; | ||
| } | ||
| p, ul { | ||
| margin: 0; | ||
| } | ||
| ul { | ||
| list-style-type: none; | ||
| padding-left: 0.5em; | ||
| margin-bottom: 0.5em; | ||
| } | ||
| li { | ||
| margin-top: 0.5em; | ||
| } | ||
| </style> | ||
| </head> | ||
| <body> | ||
| <h2>Self-hosted AMP runtime test</h2> | ||
|
|
||
| <h3>amp-geo</h3> | ||
| <p class="geo-text"><code>amp-geo</code> class successfully added to body.</p> | ||
| <p class="nogeo-text"><code>amp-geo</code> <strong>did not</strong> add a class to the body.</p> | ||
|
|
||
| <h3>amp-img</h3> | ||
| <p><amp-img layout="fixed" width="320" height="214" src="https://android.cmphys.com/temp/img/goldfish.jpg"></amp-img></p> | ||
|
|
||
| <h3>amp-form</h3> | ||
| <form method="GET" target="_top" action="self-hosted-amprt-pwa.html"> | ||
| <p>View in PWA:</p> | ||
| <ul> | ||
| <li><label><input name="page" type="radio" value="amp"> Standard AMP</label></li> | ||
| <li><label><input name="page" type="radio" value="opt"> Optimized AMP</label></li> | ||
| </ul> | ||
| <button type="submit">Submit</button> | ||
| </form> | ||
|
|
||
| <h3>View as AMP (non-PWA):</h3> | ||
| <ul> | ||
| <li><a href="self-hosted-amprt-amp.html">Standard AMP page</a></li> | ||
| <li><a href="self-hosted-amprt-opt.html">Optimized AMP page</a></li> | ||
| </ul> | ||
|
|
||
| <amp-geo layout=nodisplay> | ||
| <script type="application/json"> | ||
| { "ISOCountryGroups": { "isus": [ "us" ] } } | ||
| </script> | ||
| </amp-geo> | ||
| </body> | ||
| </html> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| <!DOCTYPE html><html ⚡="" i-amphtml-layout="" i-amphtml-no-boilerplate="" transformed="self;v=1"><head><meta charset="utf-8"><style amp-runtime="" i-amphtml-version="011910071803120">html{overflow-x:hidden!important}html.i-amphtml-fie{height:100%!important;width:100%!important}html:not([amp4ads]),html:not([amp4ads]) body{height:auto!important}html:not([amp4ads]) body{margin:0!important}body{-webkit-text-size-adjust:100%;-moz-text-size-adjust:100%;-ms-text-size-adjust:100%;text-size-adjust:100%}html.i-amphtml-singledoc.i-amphtml-embedded{-ms-touch-action:pan-y;touch-action:pan-y}html.i-amphtml-fie>body,html.i-amphtml-singledoc>body{overflow:visible!important}html.i-amphtml-fie:not(.i-amphtml-inabox)>body,html.i-amphtml-singledoc:not(.i-amphtml-inabox)>body{position:relative!important}html.i-amphtml-webview>body{overflow-x:hidden!important;overflow-y:visible!important;min-height:100vh!important}html.i-amphtml-ios-embed-legacy>body{overflow-x:hidden!important;overflow-y:auto!important;position:absolute!important}html.i-amphtml-ios-embed{overflow-y:auto!important;position:static}#i-amphtml-wrapper{overflow-x:hidden!important;overflow-y:auto!important;position:absolute!important;top:0!important;left:0!important;right:0!important;bottom:0!important;margin:0!important;display:block!important}html.i-amphtml-ios-embed.i-amphtml-ios-overscroll,html.i-amphtml-ios-embed.i-amphtml-ios-overscroll>#i-amphtml-wrapper{-webkit-overflow-scrolling:touch!important}#i-amphtml-wrapper>body{position:relative!important;border-top:1px solid transparent!important}#i-amphtml-wrapper+body{visibility:visible}#i-amphtml-wrapper+body .i-amphtml-lightbox-element,#i-amphtml-wrapper+body[i-amphtml-lightbox]{visibility:hidden}#i-amphtml-wrapper+body[i-amphtml-lightbox] .i-amphtml-lightbox-element{visibility:visible}#i-amphtml-wrapper.i-amphtml-scroll-disabled,.i-amphtml-scroll-disabled{overflow-x:hidden!important;overflow-y:hidden!important}amp-instagram{padding:54px 0px 0px!important;background-color:#fff}amp-iframe iframe{box-sizing:border-box!important}[amp-access][amp-access-hide]{display:none}[subscriptions-dialog],body:not(.i-amphtml-subs-ready) [subscriptions-action],body:not(.i-amphtml-subs-ready) [subscriptions-section]{display:none!important}amp-experiment,amp-live-list>[update],amp-share-tracking{display:none}.i-amphtml-jank-meter{position:fixed;background-color:rgba(232,72,95,0.5);bottom:0;right:0;color:#fff;font-size:16px;z-index:1000;padding:5px}amp-list[resizable-children]>.i-amphtml-loading-container.amp-hidden{display:none!important}amp-list[load-more] [load-more-button],amp-list[load-more] [load-more-end],amp-list[load-more] [load-more-failed],amp-list[load-more] [load-more-loading]{display:none}amp-story-page,amp-story[standalone]{min-height:1px!important;display:block!important;height:100%!important;margin:0!important;padding:0!important;overflow:hidden!important;width:100%!important}amp-story[standalone]{background-color:#202125!important;position:relative!important}amp-story-page{background-color:#757575}amp-story .i-amphtml-loader{display:none!important}amp-story-page:not(:first-of-type):not([distance]):not([active]){transform:translateY(1000vh)!important}amp-autocomplete{position:relative!important;display:inline-block!important}amp-autocomplete>input{padding:.5rem;border:1px solid rgba(0,0,0,0.33)}.i-amphtml-autocomplete-results,amp-autocomplete>input{font-size:1rem;line-height:1.5rem}[amp-fx^=fly-in]{visibility:hidden} | ||
| /*# sourceURL=/css/ampdoc.css*/[hidden]{display:none!important}.i-amphtml-element{display:inline-block}.i-amphtml-blurry-placeholder{transition:opacity 0.3s cubic-bezier(0.0,0.0,0.2,1)!important}[layout=nodisplay]:not(.i-amphtml-element){display:none!important}.i-amphtml-layout-fixed,[layout=fixed][width][height]:not(.i-amphtml-layout-fixed){display:inline-block;position:relative}.i-amphtml-layout-responsive,[layout=responsive][width][height]:not(.i-amphtml-layout-responsive),[width][height][sizes]:not(.i-amphtml-layout-responsive){display:block;position:relative}.i-amphtml-layout-intrinsic{display:inline-block;position:relative;max-width:100%}.i-amphtml-intrinsic-sizer{max-width:100%;display:block!important}.i-amphtml-layout-container,.i-amphtml-layout-fixed-height,[layout=container],[layout=fixed-height][height]{display:block;position:relative}.i-amphtml-layout-fill,[layout=fill]:not(.i-amphtml-layout-fill){display:block;overflow:hidden!important;position:absolute;top:0;left:0;bottom:0;right:0}.i-amphtml-layout-flex-item,[layout=flex-item]:not(.i-amphtml-layout-flex-item){display:block;position:relative;-ms-flex:1 1 auto;flex:1 1 auto}.i-amphtml-layout-fluid{position:relative}.i-amphtml-layout-size-defined{overflow:hidden!important}.i-amphtml-layout-awaiting-size{position:absolute!important;top:auto!important;bottom:auto!important}i-amphtml-sizer{display:block!important}.i-amphtml-blurry-placeholder,.i-amphtml-fill-content{display:block;height:0;max-height:100%;max-width:100%;min-height:100%;min-width:100%;width:0;margin:auto}.i-amphtml-layout-size-defined .i-amphtml-fill-content{position:absolute;top:0;left:0;bottom:0;right:0}.i-amphtml-layout-intrinsic .i-amphtml-sizer{max-width:100%}.i-amphtml-replaced-content,.i-amphtml-screen-reader{padding:0!important;border:none!important}.i-amphtml-screen-reader{position:fixed!important;top:0px!important;left:0px!important;width:4px!important;height:4px!important;opacity:0!important;overflow:hidden!important;margin:0!important;display:block!important;visibility:visible!important}.i-amphtml-screen-reader~.i-amphtml-screen-reader{left:8px!important}.i-amphtml-screen-reader~.i-amphtml-screen-reader~.i-amphtml-screen-reader{left:12px!important}.i-amphtml-screen-reader~.i-amphtml-screen-reader~.i-amphtml-screen-reader~.i-amphtml-screen-reader{left:16px!important}.i-amphtml-unresolved{position:relative;overflow:hidden!important}.i-amphtml-select-disabled{-webkit-user-select:none!important;-moz-user-select:none!important;-ms-user-select:none!important;user-select:none!important}.i-amphtml-notbuilt,[layout]:not(.i-amphtml-element){position:relative;overflow:hidden!important;color:transparent!important}.i-amphtml-notbuilt:not(.i-amphtml-layout-container)>*,[layout]:not([layout=container]):not(.i-amphtml-element)>*{display:none}.i-amphtml-ghost{visibility:hidden!important}.i-amphtml-element>[placeholder],[layout]:not(.i-amphtml-element)>[placeholder]{display:block}.i-amphtml-element>[placeholder].amp-hidden,.i-amphtml-element>[placeholder].hidden{visibility:hidden}.i-amphtml-element:not(.amp-notsupported)>[fallback],.i-amphtml-layout-container>[placeholder].amp-hidden,.i-amphtml-layout-container>[placeholder].hidden{display:none}.i-amphtml-layout-size-defined>[fallback],.i-amphtml-layout-size-defined>[placeholder]{position:absolute!important;top:0!important;left:0!important;right:0!important;bottom:0!important;z-index:1}.i-amphtml-notbuilt>[placeholder]{display:block!important}.i-amphtml-hidden-by-media-query{display:none!important}.i-amphtml-element-error{background:red!important;color:#fff!important;position:relative!important}.i-amphtml-element-error:before{content:attr(error-message)}i-amp-scroll-container,i-amphtml-scroll-container{position:absolute;top:0;left:0;right:0;bottom:0;display:block}i-amp-scroll-container.amp-active,i-amphtml-scroll-container.amp-active{overflow:auto;-webkit-overflow-scrolling:touch}.i-amphtml-loading-container{display:block!important;pointer-events:none;z-index:1}.i-amphtml-notbuilt>.i-amphtml-loading-container{display:block!important}.i-amphtml-loading-container.amp-hidden{visibility:hidden}.i-amphtml-loader-line{position:absolute;top:0;left:0;right:0;height:1px;overflow:hidden!important;background-color:hsla(0,0%,59.2%,0.2);display:block}.i-amphtml-loader-moving-line{display:block;position:absolute;width:100%;height:100%!important;background-color:hsla(0,0%,59.2%,0.65);z-index:2}@keyframes i-amphtml-loader-line-moving{0%{transform:translateX(-100%)}to{transform:translateX(100%)}}.i-amphtml-loader-line.amp-active .i-amphtml-loader-moving-line{animation:i-amphtml-loader-line-moving 4s ease infinite}.i-amphtml-loader{position:absolute;display:block;height:10px;top:50%;left:50%;transform:translateX(-50%) translateY(-50%);transform-origin:50% 50%;white-space:nowrap}.i-amphtml-loader.amp-active .i-amphtml-loader-dot{animation:i-amphtml-loader-dots 2s infinite}.i-amphtml-loader-dot{position:relative;display:inline-block;height:10px;width:10px;margin:2px;border-radius:100%;background-color:rgba(0,0,0,0.3);box-shadow:2px 2px 2px 1px rgba(0,0,0,0.2);will-change:transform}.i-amphtml-loader .i-amphtml-loader-dot:first-child{animation-delay:0s}.i-amphtml-loader .i-amphtml-loader-dot:nth-child(2){animation-delay:.1s}.i-amphtml-loader .i-amphtml-loader-dot:nth-child(3){animation-delay:.2s}@keyframes i-amphtml-loader-dots{0%,to{transform:scale(.7);background-color:rgba(0,0,0,0.3)}50%{transform:scale(.8);background-color:rgba(0,0,0,0.5)}}.i-amphtml-element>[overflow]{cursor:pointer;position:relative;z-index:2;visibility:hidden}.i-amphtml-element>[overflow].amp-visible{visibility:visible}template{display:none!important}.amp-border-box,.amp-border-box *,.amp-border-box :after,.amp-border-box :before{box-sizing:border-box}amp-pixel{display:none!important}amp-analytics,amp-story-auto-ads{position:fixed!important;top:0!important;width:1px!important;height:1px!important;overflow:hidden!important;visibility:hidden}html.i-amphtml-fie>amp-analytics{position:initial!important}[visible-when-invalid]:not(.visible),amp-list [fetch-error],form [submit-error],form [submit-success],form [submitting]{display:none}amp-accordion{display:block!important}amp-accordion>section{float:none!important}amp-accordion>section>*{float:none!important;display:block!important;overflow:hidden!important;position:relative!important}amp-accordion,amp-accordion>section{margin:0}amp-accordion>section>:last-child{display:none!important}amp-accordion>section[expanded]>:last-child{display:block!important} | ||
| /*# sourceURL=/css/ampshared.css*/</style><meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1"><link rel="preload" href="https://android.cmphys.com/temp/amprt/v0.js" as="script"><script async="" src="https://android.cmphys.com/temp/amprt/v0.js"></script><script async="" custom-element="amp-geo" src="https://cdn.ampproject.org/rtv/011910071803120/v0/amp-geo-0.1.js"></script><script async="" custom-element="amp-form" src="https://android.cmphys.com/temp/amprt/v0/amp-form-0.1.js"></script><style amp-custom="">body{margin:0;padding:8px;font-family:sans-serif;}.geo-text,body[class*="amp-geo"] .nogeo-text{display:none;}.nogeo-text,body[class*="amp-geo"] .geo-text{display:block;}h3{margin-bottom:0.5em;}p,ul{margin:0;}ul{list-style-type:none;padding-left:0.5em;margin-bottom:0.5em;}li{margin-top:0.5em;}</style><title>Self-hosted AMP test</title><link rel="canonical" href="https://www/"></head> | ||
| <body> | ||
| <h2>Self-hosted AMP runtime test</h2> | ||
|
|
||
| <h3>amp-geo</h3> | ||
| <p class="geo-text"><code>amp-geo</code> class successfully added to body.</p> | ||
| <p class="nogeo-text"><code>amp-geo</code> <strong>did not</strong> add a class to the body.</p> | ||
|
|
||
| <h3>amp-img</h3> | ||
| <p><amp-img layout="fixed" width="320" height="214" src="https://android.cmphys.com/temp/img/goldfish.jpg" class="i-amphtml-layout-fixed i-amphtml-layout-size-defined" style="width:320px;height:214px;" i-amphtml-layout="fixed"></amp-img></p> | ||
|
|
||
| <h3>amp-form</h3> | ||
| <form method="GET" target="_top" action="self-hosted-amprt-pwa.html"> | ||
| <p>View in PWA:</p> | ||
| <ul> | ||
| <li><label><input name="page" type="radio" value="amp"> Standard AMP</label></li> | ||
| <li><label><input name="page" type="radio" value="opt"> Optimized AMP</label></li> | ||
| </ul> | ||
| <button type="submit">Submit</button> | ||
| </form> | ||
|
|
||
| <h3>View as AMP (non-PWA):</h3> | ||
| <ul> | ||
| <li><a href="self-hosted-amprt-amp.html">Standard AMP page</a></li> | ||
| <li><a href="self-hosted-amprt-opt.html">Optimized AMP page</a></li> | ||
| </ul> | ||
|
|
||
| <amp-geo layout="nodisplay" class="i-amphtml-layout-nodisplay" hidden="hidden" i-amphtml-layout="nodisplay"> | ||
| <script type="application/json"> | ||
| { "ISOCountryGroups": { "isus": [ "us" ] } } | ||
| </script> | ||
| </amp-geo> | ||
|
|
||
|
|
||
| </body></html> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| <!doctype html> | ||
| <html> | ||
| <head> | ||
| <title>Self-hosted PWA test</title> | ||
| <meta charset="utf-8"> | ||
| <meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1"> | ||
| <script> | ||
| window.AMP_CONFIG = { | ||
| cdnUrl: 'https://android.cmphys.com/temp/amprt', | ||
| useDefaultCdnForDynamicComponents: true | ||
| }; | ||
| </script> | ||
| <script async src="https://android.cmphys.com/temp/amprt/shadow-v0.js"></script> | ||
| <style> | ||
| body { | ||
| margin: 0; | ||
| padding: 0; | ||
| } | ||
| </style> | ||
| </head> | ||
| <body> | ||
| <div id="shadowHost"></div> | ||
| <script> | ||
| (window.AMP = window.AMP || []).push(function(AMP) { | ||
| const page = /[?&]page=opt/.test(location.search) ? '-opt' : '-amp'; | ||
| const url = location.href.replace('-pwa', page); | ||
| const xhr = new XMLHttpRequest(); | ||
| xhr.open('GET', url); | ||
| xhr.responseType = 'document'; | ||
| xhr.onreadystatechange = function () { | ||
| if (xhr.readyState === XMLHttpRequest.DONE) { | ||
| if (xhr.status === 200) { | ||
| const hostElement = document.querySelector('#shadowHost'); | ||
| window.shadowDoc = AMP.attachShadowDoc(hostElement, xhr.response, url); | ||
| } | ||
| } | ||
| }; | ||
| xhr.send(); | ||
| }); | ||
| </script> | ||
| </body> | ||
| </html> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,20 +28,44 @@ import {urls} from '../config'; | |
| */ | ||
| let ExtensionInfoDef; | ||
|
|
||
| /** | ||
| * Dynamic AMP components | ||
| */ | ||
| const AMP_DYNAMIC_COMPONENTS = ['amp-geo']; | ||
|
There was a problem hiding this comment. This hardcoded list (now in the base runtime) is disconnected from the source of truth that makes a component "dynamic." Are there many others that are expected to fall into this category? What happens when they do? There was a problem hiding this comment. I don't know the future of how many components will become "dynamic components". I'm hesitant to assume it will always be just
I agree with your 'source of truth' comment. This probably isn't the best place for this. Would it make more sense to introduce There was a problem hiding this comment. This is resolved in commit d76da0c. Let me know how the changes look to you. |
||
|
|
||
| /** | ||
| * Determine whether an extension is a dynamic AMP component | ||
| * @param {string} extensionId | ||
| * @return {boolean} | ||
| */ | ||
| function isDynamicComponent(extensionId) { | ||
| return ( | ||
| Boolean(extensionId) && AMP_DYNAMIC_COMPONENTS.indexOf(extensionId) >= 0 | ||
|
There was a problem hiding this comment. Is case sensitivity important? E.g. There was a problem hiding this comment. I'd argue this is a correct implementation. Fuzzy matching on an ID feels sloppy. There was a problem hiding this comment. Commit cc4f0ed allows fuzzy matching. |
||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Calculate the base url for any scripts. | ||
| * @param {!Location} location The window's location | ||
| * @param {boolean=} opt_isLocalDev | ||
| * @param {boolean=} opt_useDefaultCdn | ||
| * @return {string} | ||
| */ | ||
| export function calculateScriptBaseUrl( | ||
| location, | ||
| opt_isLocalDev, | ||
| opt_useDefaultCdn | ||
| ) { | ||
| if (opt_isLocalDev) { | ||
|
There was a problem hiding this comment. As a future note @erwinmombay -- this is the kind of logic we should remove (local dev) when we support a development build. There was a problem hiding this comment. just to clarify, the development will essentially be running in its own pipeline process to separate this out correct? (which will of course clean up these logical branchings) |
||
| let prefix = `${location.protocol}//${location.host}`; | ||
| if (location.protocol == 'about:') { | ||
| prefix = ''; | ||
| } | ||
| return `${prefix}/dist`; | ||
| } | ||
| if (opt_useDefaultCdn) { | ||
| return urls.defaultCdn; | ||
| } | ||
| return urls.cdn; | ||
| } | ||
|
|
||
|
|
@@ -68,14 +92,19 @@ export function calculateExtensionScriptUrl( | |
| opt_extensionVersion, | ||
| opt_isLocalDev | ||
| ) { | ||
| const useDefaultCdn = | ||
| urls.useDefaultCdnForDynamicComponents && isDynamicComponent(extensionId); | ||
| const base = calculateScriptBaseUrl(location, opt_isLocalDev, useDefaultCdn); | ||
| const rtv = getMode().rtvVersion; | ||
| if (opt_extensionVersion == null) { | ||
| opt_extensionVersion = '0.1'; | ||
| } | ||
| const extensionVersion = opt_extensionVersion | ||
| ? '-' + opt_extensionVersion | ||
| : ''; | ||
| if (urls.cdn !== urls.defaultCdn && !useDefaultCdn) { | ||
|
There was a problem hiding this comment. If single pass is enabled does this logic still function as expected? For both conditions. There was a problem hiding this comment. Ah, I mistakenly assumed single pass mode was related to localdev, not experiments. I will rewrite this to include There was a problem hiding this comment. This is resolved in commit d76da0c. Let me know how the changes look to you. There was a problem hiding this comment. if i'm understanding it correctly d76da0c could create a path that looks like We can actually remove that feel free to ping me if you need further help on this part! (or ping me on slack too if you want a quicker response time There was a problem hiding this comment. just to make my explanation simpler, the whole There was a problem hiding this comment. @erwinmombay Since removal of single pass experiment paths is unrelated to this PR topic, would you mind handling that change in a separate commit? There was a problem hiding this comment. @mattwomple will do. I'll take it on today. 👍 There was a problem hiding this comment. I'm not sure this is safe to do. It might break RTV pathing for Bing's cache for example. Instead of inferring when to disable RTV pathing, we should probably make it an explicit option in config.js. There was a problem hiding this comment. Would you mind following-up with @sebastianbenz about this? Here's our discussion: ampproject/amp-toolbox#517 (it's worth viewing the edit history on the original post there, too). If we did not make the right call in dropping '/rtv/{rtv}' for alternate CDNs, I'd like to make sure optimizer in amp-toolbox is updated again. There was a problem hiding this comment. Hm, what's the use case for There was a problem hiding this comment.
There was a problem hiding this comment. Ah thanks, I misread what it does. I think the optimizer case it would be confusing to have both |
||
| return `${base}/v0/${extensionId}${extensionVersion}.js`; | ||
| } | ||
| const spPath = getSinglePassExperimentPath(); | ||
| return `${base}/rtv/${rtv}/${spPath}v0/${extensionId}${extensionVersion}.js`; | ||
| } | ||
|
|
@@ -96,7 +125,7 @@ export function calculateEntryPointScriptUrl( | |
| opt_rtv | ||
| ) { | ||
| const base = calculateScriptBaseUrl(location, isLocalDev); | ||
| if (opt_rtv && urls.cdn === urls.defaultCdn) { | ||
| const spPath = getSinglePassExperimentPath(); | ||
| return `${base}/rtv/${getMode().rtvVersion}/${spPath}${entryPoint}.js`; | ||
| } | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we simplify this API? I think just adding one new field should suffice:
This would allow self-hosters to use caches other than Google's for
amp-geo.It also avoids introducing "default" and "dynamic component" both of which need explaining. AFAIK there's no plan for other dynamic components so we can cross that bridge when we get there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course I'll be happy to make this change if it really is preferred, but if I were in your shoes and saw someone submit a PR to introduce
cdnForAmpGeo, I'd think "gosh, that's a little short sighted".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or
cdnForCacheModifiedExtensionssince this is public. :)"Dynamic" is a bit overloaded in AMP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine. And then this documentation should be updated as well to adopt the new name: https://github.com/ampproject/amphtml/blob/master/spec/amp-cache-guidelines.md#guidelines-adding-a-new-cache-to-the-amp-ecosystem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit cc4f0ed makes this change, but I didn't want to introduce a 3rd CDN URL, so I opted for
cdnSupportsCacheModifiedExtensionsboolean. Let me know what you think.