ampproject / amphtml

The AMP web component framework.
https://amp.dev
Apache License 2.0
14.9k stars 3.89k forks source link

Add a post-dist test to ensure amp-geo contains {{AMP_ISO_COUNTRY_HOTPATCH}} #35677

Open jridgewell opened 3 years ago

jridgewell commented 3 years ago

Description

Inspired by https://github.com/ampproject/amphtml/pull/35654, I wanted to see if we could remove all other toLowerCase() calls by lowercasing the COUNTRY variable (which is hot-patched by servers when serving):

diff --git a/extensions/amp-geo/0.1/amp-geo.js b/extensions/amp-geo/0.1/amp-geo.js
index 7aaea1108c..d50d9cf9cb 100644
--- a/extensions/amp-geo/0.1/amp-geo.js
+++ b/extensions/amp-geo/0.1/amp-geo.js
@@ -185,7 +185,7 @@ export class AmpGeo extends AMP.BaseElement {
     // 'xx xx-xx  ': trimmedGeoMatch is ["xx xx-xx  ", "xx", "xx-xx"];
     // '          ': trimmedGeoMatch is ["          ", undefined, undefined];
     // '{{AMP_ISO_COUNTRY_HOTPATCH}}':  ["", undefined, undefined]
-    const trimmedGeoMatch = GEO_HOTPATCH_STR_REGEX.exec(COUNTRY);
+    const trimmedGeoMatch = GEO_HOTPATCH_STR_REGEX.exec(COUNTRY.toLowerCase());

     // default country is 'unknown' which is also the zero length case
     if (
@@ -218,11 +218,8 @@ export class AmpGeo extends AMP.BaseElement {
     } else if (trimmedGeoMatch[1]) {
       // We have a valid 2 letter ISO country
       this.mode_ = mode.GEO_HOT_PATCH;
-      this.country_ = trimmedGeoMatch[1].toLowerCase();
-      if (
-        trimmedGeoMatch[2] &&
-        trimmedGeoMatch[2].toLowerCase() === US_CA_CODE
-      ) {
+      this.country_ = trimmedGeoMatch[1];
+      if (trimmedGeoMatch[2] === US_CA_CODE) {
         // Has subdivision code support (us-ca only)
         this.subdivision_ = US_CA_CODE;
       }

Looks good. But now when we run amp dist --extensions=amp-geo and inspect the output, we get:

T.exec("{{amp_iso_country_hotpatch}}");

Closure/terser has optimized a known constant '{{AMP_ISO_COUNTRY_HOTPATCH}}'.toLowerCase() (because COUNTRY is constant and statically analyzable) into '{{amp_iso_country_hotpatch}}'.

This breaks the hot-patching contract, and now our servers will fail to serve the correct geo-code to users. This isn't catchable unless it's done after the dist process, and it looks like a totally innocent change to make. That's a recipe for breaking things.

Alternatives Considered

N/a

Additional Context

No response

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.