ampproject / ampproject/amphtml
Add a post-dist test to ensure amp-geo contains {{AMP_ISO_COUNTRY_HOTPATCH}}
- 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
Assessment
This issue has not been assessed yet.