ampproject / ampproject/amphtml

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

Open
#35,677 1 comment 0 reactions 1 assignee Claimed by @rsimha View on GitHub
Stale Type: Feature Request Type: Testing
Dominant language
JavaScript
Stars
14.9k
Forks
4.1k
PR merge metrics
No merged PRs in 30d

Description

### 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
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:

```js
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_

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.