optimizer: Do not append rtv/{rtv}/ to ampUrlPrefix by mattwomple · Pull Request #517 · ampproject/amp-toolbox

mattwomple · 2019-10-04T22:02:49Z

BREAKING CHANGE

When both ampUrlPrefix and ampRuntimeVersion are specified, do not
append rtv/{rtv}/ to the prefix. If version information is expected in
the URL, it can be included in ampUrlPrefix.

sebastianbenz · 2019-10-08T13:49:59Z

Hm. This is confusing indeed. I'd prefer to not introduce a third parameter though. My suggestion would be: don't add the runtime version if a prefix is given. If someone wants to self-host a versioned runtime, they can append the version string to the prefix. Wdyt?

mattwomple · 2019-10-08T14:10:42Z

Hm. This is confusing indeed. I'd prefer to not introduce a third parameter though. My suggestion would be: don't add the runtime version if a prefix is given. If someone wants to self-host a versioned runtime, they can append the version string to the prefix. Wdyt?

This would absolutely be my preference as well. It is a breaking change, though. Anyone who has optimizer implemented using ampUrlPrefix and ampRuntimeVersion with the expectation that the default pattern /rtv/{rtv} will automatically apply to their ampUrlPrefix will suddenly find their URLs broken when amp-toolbox updates in the future. Is it early enough in amp-toolbox's life that this is ok?

sebastianbenz · 2019-10-08T14:59:51Z

Yeah. I was thinking about the same. The transformer is marked as experimental since the 1.0 release so API changes are expected. I'm also not aware of anyone combining rtv + self-hosting. So I'd be OK with this change.

googlebot · 2019-10-08T17:47:17Z

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 then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

BREAKING CHANGE When both ampUrlPrefix and ampRuntimeVersion are specified, do not append rtv/{rtv}/ to the prefix. If version information is expected in the URL, it can be included in ampUrlPrefix.

googlebot · 2019-10-08T17:51:15Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

mattwomple · 2019-10-08T17:53:30Z

Sorry for the force pushes. I accidentally committed changes using a different GitHub account.

mattwomple · 2019-10-09T14:56:16Z

Tests have been updated. This is ready for review.

sebastianbenz

One nit. Otherwise LGTM

packages/optimizer/README.md

dreamofabear · 2019-10-15T17:14:46Z

Why not ask callers to omit ampRuntimeVersion if they don't want RTV paths when using ampUrlPrefix?

mattwomple · 2019-10-15T17:22:35Z

Why not ask callers to omit ampRuntimeVersion if they don't want RTV paths when using ampUrlPrefix?

Setting ampRuntimeVersion ensures you can maintain a correspondence between self-hosted runtime version and the version used during optimization. Consider this code:

amp-toolbox/packages/optimizer/lib/transformers/AmpBoilerplateTransformer.js

Lines 85 to 90 in efe5775

    
           if (version) { 
        
             v0CssUrl = appendRuntimeVersion(AMP_CACHE_HOST, version) + '/' + V0_CSS; 
        
           } else { 
        
             v0CssUrl = V0_CSS_URL; 
        
             version = await this.runtimeVersion_.currentVersion(); 
        
           }

. If the self-hosted runtime is pushed on Monday, and then on Wednesday an AMP page is optimized, the inline CSS in the optimized AMP page will be associated with a different runtime version.

dreamofabear · 2019-10-15T17:47:07Z

Right, I was wondering specifically about the interaction between the two options.

ampRuntimeVersion only
ampUrlPrefix only
ampRuntimeVersion and ampUrlPrefix

Instead of making (3) behave like (2), why not ask callers to just do (2)?

mattwomple · 2019-10-15T17:51:23Z

Instead of making (3) behave like (2), why not ask callers to just do (2)?

How would optimizer know which runtime version should be used for performing its optimizations? It is not safe to assume "latest RTV available on cdn.ampproject.org". Specifying both ampRuntimeVersion and ampUrlPrefix informs optimizer what the URLs should look like to extensions and what version of boilerplate CSS should be included.

dreamofabear · 2019-10-15T17:55:05Z

Oh, I thought that cache transforms are not pegged to RTV. E.g. if they were, it would make AMP rollbacks very tricky.

/cc @ampproject/wg-caching

sebastianbenz · 2019-10-15T17:59:40Z

RTV is only relevant for v0.css. The AMP runtime will download the latest v0.css in case of a version mismatch.

dreamofabear · 2019-10-15T19:18:37Z

...it does? 😛

I know runtime does this for JS (v0 vs. extension RTV mismatch) but I don't think we do this for CSS which is bundled directly in the JS.

sebastianbenz · 2019-10-15T19:27:11Z

😀 ~~download~~ update. In any case, outdated v0.css shouldn't be an issue (at least that's what I was told that when I implemented the transformer).

…ct#517)" This reverts commit 9e7533c. When self-hosting, follow the same URL structure in use by cdn.ampproject.org, specifically: - Non versioned URLs: https://example.com/runtime/ - Versioned URLs: https://example.com/runtime/rtv/{rtv}/ - Runtime metadata: https://example.com/rtv/metadata

…ct#517)" This reverts commit 9e7533c.

* Revert "optimizer: Make dynamic component URL rewrites optional (#518)" This reverts commit 81e525d. * Revert "optimizer: Do not append rtv/{rtv}/ to ampUrlPrefix (#517)" This reverts commit 9e7533c.

googlebot added the cla: yes label Oct 4, 2019

mattwomple mentioned this pull request Oct 4, 2019

optimizer: Make dynamic component URL rewrites optional #518

Merged

googlebot added cla: no and removed cla: yes labels Oct 8, 2019

optimizer: Do not append rtv/{rtv}/ to ampUrlPrefix

8c56448

BREAKING CHANGE When both ampUrlPrefix and ampRuntimeVersion are specified, do not append rtv/{rtv}/ to the prefix. If version information is expected in the URL, it can be included in ampUrlPrefix.

googlebot added cla: yes and removed cla: no labels Oct 8, 2019

mattwomple changed the title ~~optimizer: Support suppression of RTV in rewrite URLs~~ optimizer: Do not append rtv/{rtv}/ to ampUrlPrefix Oct 8, 2019

optimizer: Update tests for revised ampUrlPrefix handling

29a5600

sebastianbenz approved these changes Oct 9, 2019

View reviewed changes

packages/optimizer/README.md Outdated Show resolved Hide resolved

optimizer: Revise version in ampUrlPrefix notice

79edabc

sebastianbenz added the bug label Oct 9, 2019

sebastianbenz merged commit 9e7533c into ampproject:master Oct 9, 2019

mattwomple deleted the patch-1 branch October 9, 2019 19:47

mattwomple mentioned this pull request Oct 13, 2019

Improve support for self-hosted runtime ampproject/amphtml#25026

Closed

mattwomple mentioned this pull request Dec 9, 2019

Improve boilerplate inline CSS handling when self-hosting AMP runtime #550

Closed

mdmower added a commit to mdmower/amp-toolbox that referenced this pull request Jan 29, 2020

Revert "optimizer: Do not append rtv/{rtv}/ to ampUrlPrefix (ampproje…

b6fb439

…ct#517)" This reverts commit 9e7533c.

mdmower added a commit to mdmower/amp-toolbox that referenced this pull request Feb 2, 2020

Revert "optimizer: Do not append rtv/{rtv}/ to ampUrlPrefix (ampproje…

d5a2793

…ct#517)" This reverts commit 9e7533c.

Conversation

mattwomple commented Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebastianbenz commented Oct 8, 2019

Uh oh!

mattwomple commented Oct 8, 2019

Uh oh!

sebastianbenz commented Oct 8, 2019

Uh oh!

googlebot commented Oct 8, 2019

Uh oh!

googlebot commented Oct 8, 2019

Uh oh!

mattwomple commented Oct 8, 2019

Uh oh!

mattwomple commented Oct 9, 2019

Uh oh!

sebastianbenz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dreamofabear commented Oct 15, 2019

Uh oh!

mattwomple commented Oct 15, 2019

Uh oh!

dreamofabear commented Oct 15, 2019

Uh oh!

mattwomple commented Oct 15, 2019

Uh oh!

dreamofabear commented Oct 15, 2019

Uh oh!

sebastianbenz commented Oct 15, 2019

Uh oh!

dreamofabear commented Oct 15, 2019

Uh oh!

sebastianbenz commented Oct 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mattwomple commented Oct 4, 2019 •

edited

Loading