optimizer: Make dynamic component URL rewrites optional by mattwomple · Pull Request #518 · ampproject/amp-toolbox

mattwomple · 2019-10-04T23:44:13Z

DEPENDS ON #517

Not everyone who self-hosts the AMP runtime has the infrastructure to
support dynamic components like amp-geo, ref: AMP cache guidelines.

Introduce option rewriteDynamicComponents that prevents modification of
dynamic component URLs that would otherwise have ampUrlPrefix
substituted.

Two relevant tests added:

rewrites_host_but_not_dynamic_components_with_rtv
rewrites_host_but_not_dynamic_components_without_rtv

Fixes #515

mattwomple · 2019-10-09T15:52:26Z

This is ready for review, with the caveat that it has been written with #517 as its base.

sebastianbenz

Looks good except: I think this behavior should be supported by versioned and unversioned runtime artifacts (or am I missing anything?).

Otherwise few nits.

sebastianbenz · 2019-10-09T17:40:24Z

packages/optimizer/README.md

+
+  **Notice:** The behavior of `ampUrlPrefix` when used in conjunction with `ampRuntimeVersion` changed beginning with version 2.0. Prior to 2.0, `rtv/{rtv}/` was automatically appended to `ampUrlPrefix` when `ampRuntimeVersion` was specified. Since version 2.0, `ampUrlPrefix` is not modified when `ampRuntimeVersion` is also specified.
+
+- `rewriteDynamicComponents`: When used in conjunction with `ampUrlPrefix`, this option can be set to `false` to prevent [dynamic AMP components](https://github.com/ampproject/amphtml/blob/master/spec/amp-cache-guidelines.md#guidelines-adding-a-new-cache-to-the-amp-ecosystem) from having their URLs rewritten. This option is only effective if `ampRuntimeVersion` is specified. This ensures the self-hosted runtime version matches version of the dynamic component delivered by `cdn.ampproject.org`.


This option is only effective if ampRuntimeVersion is specified. This ensures the self-hosted runtime version matches version of the dynamic component delivered by cdn.ampproject.org.

I don't understand why it only makes sense in conjunction with ampRuntime Version.

This is resolved in commit 3354b6a.

sebastianbenz · 2019-10-09T17:41:10Z

packages/optimizer/lib/transformers/RewriteAmpUrls.js

+ * re-written. You can combine `ampRuntimeVersion` and `ampUrlPrefix` to rewrite
+ * AMP runtime URLs to versioned URLs on a different origin.
+ *
+ * rewriteDynamicComponents is only effective if `ampRuntimeVersion` and


same as above

This is resolved in commit 3354b6a.

sebastianbenz · 2019-10-09T17:41:42Z

packages/optimizer/lib/transformers/RewriteAmpUrls.js


+    // Should be kept up to date with dynamic components listed here:
+    // https://github.com/ampproject/amphtml/blob/master/spec/amp-cache-guidelines.md#guidelines-adding-a-new-cache-to-the-amp-ecosystem
+    const dynamicElements = ['amp-geo'];


please move this into a constant at the top of the file

I added it to AMP_CONSTANTS. Good? Bad?

sebastianbenz · 2019-10-09T17:41:49Z

packages/optimizer/lib/transformers/RewriteAmpUrls.js

+    // Should be kept up to date with dynamic components listed here:
+    // https://github.com/ampproject/amphtml/blob/master/spec/amp-cache-guidelines.md#guidelines-adding-a-new-cache-to-the-amp-ecosystem
+    const dynamicElements = ['amp-geo'];
+    const dynamicTemplates = [];


same as above

I added it to AMP_CONSTANTS. Good? Bad?

even better

sebastianbenz · 2019-10-09T17:43:28Z

packages/optimizer/lib/transformers/RewriteAmpUrls.js

    while (node) {
      if (node.tagName === 'script' && this._usesAmpCacheUrl(node.attribs.src)) {
-        node.attribs.src = this._replaceUrl(node.attribs.src, ampUrlPrefix);
+        const isDynamicComponent =


please extract this into a separate method for readability

Let me know if commit 3354b6a does this adequately.

mattwomple · 2019-10-09T17:47:35Z

Looks good except: I think this behavior should be supported by versioned and unversioned runtime artifacts (or am I missing anything?).

See here and my following comment: #515 (comment)

sebastianbenz · 2019-10-09T18:03:28Z

Hah! Completely forgot about this 🤦‍♂️. It starts getting tricky. With the revised self-hosting behavior (ampRuntimeVersion no longer automatically being appended) the version will no longer be automatically enforced. However, the good news is: the AMP runtime will automatically download the matching component version if there's a mismatch. So worst case is: the extension needs to be downloaded twice, but will continue to work. That's acceptable for me.

Hence, I'd favor making this independent of the runtime version.

Not everyone who self-hosts the AMP runtime has the infrastructure to support dynamic components like amp-geo, ref: https://github.com/ampproject/amphtml/blob/master/spec/amp-cache-guidelines.md#guidelines-adding-a-new-cache-to-the-amp-ecosystem Introduce option rewriteDynamicComponents that prevents modification of dynamic component URLs that would otherwise have ampUrlPrefix substituted. Two relevant tests added: rewrites_host_but_not_dynamic_components_with_rtv rewrites_host_but_not_dynamic_components_without_rtv

sebastianbenz

Thanks a lot!

…roject#518)" This reverts commit 81e525d. Do not allow "partial self-hosting". All AMP components should come from the same origin to ensure version locking support is maintained. An alternative solution for amp-geo is to fall back to an API request instead of requiring cache modification when amp-geo-0.1.js is served.

…roject#518)" This reverts commit 81e525d.

* 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 changed the title ~~[wip] optimizer: Make dynamic component URL rewrites optional~~ optimizer: Make dynamic component URL rewrites optional Oct 9, 2019

sebastianbenz requested changes Oct 9, 2019

View reviewed changes

sebastianbenz added the enhancement label Oct 9, 2019

sebastianbenz approved these changes Oct 9, 2019

View reviewed changes

sebastianbenz merged commit 81e525d into ampproject:master Oct 9, 2019

mattwomple deleted the patch-2 branch October 9, 2019 19:55

mattwomple mentioned this pull request Oct 13, 2019

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

Closed

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

Revert "optimizer: Make dynamic component URL rewrites optional (ampp…

10e6197

…roject#518)" This reverts commit 81e525d.

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

Revert "optimizer: Make dynamic component URL rewrites optional (ampp…

b54b328

…roject#518)" This reverts commit 81e525d.


		Notice: The behavior of `ampUrlPrefix` when used in conjunction with `ampRuntimeVersion` changed beginning with version 2.0. Prior to 2.0, `rtv/{rtv}/` was automatically appended to `ampUrlPrefix` when `ampRuntimeVersion` was specified. Since version 2.0, `ampUrlPrefix` is not modified when `ampRuntimeVersion` is also specified.

		- `rewriteDynamicComponents`: When used in conjunction with `ampUrlPrefix`, this option can be set to `false` to prevent [dynamic AMP components](https://github.com/ampproject/amphtml/blob/master/spec/amp-cache-guidelines.md#guidelines-adding-a-new-cache-to-the-amp-ecosystem) from having their URLs rewritten. This option is only effective if `ampRuntimeVersion` is specified. This ensures the self-hosted runtime version matches version of the dynamic component delivered by `cdn.ampproject.org`.

Conversation

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

Uh oh!

Uh oh!

mattwomple commented Oct 9, 2019

Uh oh!

sebastianbenz left a comment

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!

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!

mattwomple commented Oct 9, 2019

Uh oh!

sebastianbenz commented Oct 9, 2019

Uh oh!

sebastianbenz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mattwomple commented Oct 4, 2019 •

edited

Loading