search

ampproject / amphtml

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

New issue: GEONOTPATCHED: amp-geo served unpatched #20392

Closed adelinamart closed 5 years ago

adelinamart commented 5 years ago

Error: GEONOTPATCHED: amp-geo served unpatched, ISO country not set at Error (https://raw.githubusercontent.com/ampproject/amphtml/1901151846530/src/log.js:540) at apply (https://raw.githubusercontent.com/ampproject/amphtml/1901151846530/src/log.js:244) at apply (https://raw.githubusercontent.com/ampproject/amphtml/1901151846530/src/log.js:257) at error (https://raw.githubusercontent.com/ampproject/amphtml/1901151846530/extensions/amp-geo/0.1/amp-geo.js:204) at findCountry_ (https://raw.githubusercontent.com/ampproject/amphtml/1901151846530/extensions/amp-geo/0.1/amp-geo.js:296)

Service default-cdn-1p-canary Version 001901151846530

jpettitt commented 5 years ago

I'm not seeing this error on amp by example (same canary build).

The error should only happen if amp-geo is served unpatched by our serving infrastructure.

jpettitt commented 5 years ago

Tracked internally at Google b/123012667

jridgewell commented 5 years ago

Due to #20015. You've imported a file file (extensions/amp-geo/0.1/amp-geo.js) which contains impure code (https://github.com/ampproject/amphtml/blob/master/extensions/amp-geo/0.1/amp-geo.js#L407-L411). That code will run in whatever binaries import it.

So, amp-consent.js now contains amp-geo's custom element. amp-consent.js also contains {{AMP_ISO_COUNTRY_HOTPATCH}}, which won't be hotpached because isn't not the amp-geo.js file. Now we have a race to see whether amp-consent.js will download first, or amp-geo.js.

adelinamart commented 5 years ago

Please let us know if this is a blocker for next week production. Thanks.

jridgewell commented 5 years ago

It is, we'll need a new canary cut.

adelinamart commented 5 years ago

okay. Please open a cherry pick issue. Thanks.

adelinamart commented 5 years ago

I'm still seeing the issue in the new canary release

jpettitt commented 5 years ago

@adelinamart can you confirm you are seeing it with 190118170051 ? I'm not seeing the double inclusion in that version.

jpettitt commented 5 years ago

Looks like there is a separate, low volume, issue where amp-geo-latest.js is not being patched but amp-geo-0.1.js is. This is a serving infrastructure issue. fixed in cl/230336455

gmajoulet commented 5 years ago

Should we wait for this internal CL to be submitted before promoting the canary to prod?

jpettitt commented 5 years ago

The CL doesn't change anything in the actual build it only changes serving infrastructure. I don't see a blocker on promoting - @jridgewell do you concur?

jpettitt commented 5 years ago

Fix is working, there are some lingering cached files which will clear with the new canary build today. (the 01 build is working so promoting the current canary is safe)

jpettitt commented 5 years ago

We're no longer seeing this error - closing the issue. Any recurrence should be treated as a new issue as it will almost certainly have a different root cause.