Amphtml: Lots of `Error: GEONOTPATCHED: amp-geo served unpatched, ISO country not set` in latest Canary

[aghassemi]

There are lots of new Error: GEONOTPATCHED: amp-geo served unpatched, ISO country not set errors in the new Canary.

go/amp-error/CJOu-fOv5uiPzwE

I am reverting 1% because of it.

[jridgewell]

The trim() was optimized away, likely in the closure upgrades.

[aghassemi]

What's happening here is that a trim() call that is performed after amp-geo replaces the magic string is being optimize out by the new version of the closure compiler, so likely caused by https://github.com/ampproject/amphtml/pull/21618

Prod (without bug)

Notice the trim and the end of "us " string

Canary (with bug)

Notice the trim() is missing which means we end up with the wrong value.

Root cause

I think new version of CC added in https://github.com/ampproject/amphtml/pull/21618 is being super smart here an removing trim() because the magic string and later gets replaced at serving time is const COUNTRY = '{{AMP_ISO_COUNTRY_HOTPATCH}}'; and {{AMP_ISO_COUNTRY_HOTPATCH}} doesn't need trimming.

[aghassemi]

@jridgewell beat me to it.

[aghassemi]

We can either not pad the country code and break source map or try to outsmart CC (until they outsmart us at somepoint in the future)

Looking at CC's optimizations around string method , it doesn't look like they try to optimize replace, only trim and upper/lowerCase. So s/trim()/.replace(' ','')? there is no whitespace in the middle to worry about. WDYT @jridgewell?

[jridgewell]

I'm putting together a regex now, which can never be optimized.