Refactor amp-geo to use externs, (ampproject#20015)

* Refactor amp-geo to use externs,
update dependant extension to use isInCountryGroup()

* isInGeoGroup() returns enum.

* bad path

* revert externs

* remove stray newline

* change config to prevent single pass closure mangle

* add new owners

extensions/amp-consent/0.1/amp-consent.js

@@ -21,6 +21,7 @@ import {ConsentPolicyManager} from './consent-policy-manager';
 import {ConsentStateManager} from './consent-state-manager';
 import {ConsentUI} from './consent-ui';
 import {Deferred} from '../../../src/utils/promise';
+import {GEO_IN_GROUP} from '../../amp-geo/0.1/amp-geo';
 import {
   NOTIFICATION_UI_MANAGER,
   NotificationUiManager,
@@ -451,7 +452,7 @@ export class AmpConsent extends AMP.BaseElement {
     return Services.geoForDocOrNull(this.element).then(geo => {
       userAssert(geo,
           'requires <amp-geo> to use promptIfUnknownForGeoGroup');
-      return (geo.ISOCountryGroups.indexOf(geoGroup) >= 0);
+      return (geo.isInCountryGroup(geoGroup) == GEO_IN_GROUP.IN);
     });
   }
 

extensions/amp-consent/0.1/test/test-amp-consent.js

@@ -19,6 +19,7 @@ import {
   AmpConsent,
 } from '../amp-consent';
 import {CONSENT_ITEM_STATE} from '../consent-info';
+import {GEO_IN_GROUP} from '../../../amp-geo/0.1/amp-geo';
 import {dict} from '../../../../src/utils/object';
 import {macroTask} from '../../../../testing/yield';
 import {
@@ -75,7 +76,9 @@ describes.realWin('amp-consent', {
     resetServiceForTesting(win, 'geo');
     registerServiceBuilder(win, 'geo', function() {
       return Promise.resolve({
-        'ISOCountryGroups': ISOCountryGroups,
+        isInCountryGroup: group =>
+          ISOCountryGroups.indexOf(group) >= 0 ?
+            GEO_IN_GROUP.IN : GEO_IN_GROUP.NOT_IN,
       });
     });
 

extensions/amp-geo/0.1/amp-geo.js

@@ -91,15 +91,15 @@ const mode = {
   GEO_OVERRIDE: 2, //  We've been overriden in test by #amp-geo=xx
 };
 
+
 /**
- * @typedef {{
- *   ISOCountry: string,
- *   matchedISOCountryGroups: !Array<string>,
- *   allISOCountryGroups: !Array<string>,
- *   isInCountryGroup: GEO_IN_GROUP,
- *   ISOCountryGroups: !Array<string>
- * }}
- */
+  * @typedef {{
+  *   ISOCountry: string,
+  *   matchedISOCountryGroups: !Array<string>,
+  *   allISOCountryGroups: !Array<string>,
+  *   isInCountryGroup: (function(string):GEO_IN_GROUP),
+  * }}
+  */
 export let GeoDef;
 
 
@@ -213,7 +213,7 @@ export class AmpGeo extends AMP.BaseElement {
   matchCountryGroups_(config) {
     // ISOCountryGroups are optional but if specified at least one must exist
     const ISOCountryGroups = /** @type {!Object<string, !Array<string>>} */(
-      config.ISOCountryGroups);
+      config['ISOCountryGroups']);
     const errorPrefix = '<amp-geo> ISOCountryGroups'; // code size
     if (ISOCountryGroups) {
       this.assertWithErrorReturn_(
@@ -336,7 +336,7 @@ export class AmpGeo extends AMP.BaseElement {
 
             // Only include amp state if user requests it to
             // avoid validator issue with missing amp-bind js
-            if (config.AmpBind) {
+            if (config['AmpBind']) {
               const geoState = doc.getElementById(GEO_ID);
               if (geoState) {
                 geoState.parentNode.removeChild(geoState);
@@ -363,13 +363,6 @@ export class AmpGeo extends AMP.BaseElement {
         allISOCountryGroups: this.definedGroups_,
         /* API */
         isInCountryGroup: this.isInCountryGroup.bind(this),
-        /**
-         * Temp still return old interface to avoid version skew
-         * with consuming extensions.  This will go away don't use it!
-         * replace with matchedISOCountryGroups or use the isInCountryGroup
-         * API
-         */
-        ISOCountryGroups: self.matchedGroups_,
       };
     });
   }

extensions/amp-geo/OWNERS.yaml

@@ -1 +1,3 @@
 - jpettitt
+- zhoux
+- lannka

extensions/amp-user-notification/0.1/amp-user-notification.js

@@ -16,6 +16,7 @@
 
 import {CSS} from '../../../build/amp-user-notification-0.1.css';
 import {Deferred} from '../../../src/utils/promise';
+import {GEO_IN_GROUP} from '../../amp-geo/0.1/amp-geo';
 import {
   NOTIFICATION_UI_MANAGER,
   NotificationUiManager,
@@ -217,7 +218,7 @@ export class AmpUserNotification extends AMP.BaseElement {
           'requires <amp-geo> to use promptIfUnknownForGeoGroup');
 
       const matchedGeos = geoGroup.split(/,\s*/).filter(group => {
-        return geo.ISOCountryGroups.indexOf(group) >= 0;
+        return geo.isInCountryGroup(group) == GEO_IN_GROUP.IN;
       });
 
       // Invert if includeGeos is false

extensions/amp-user-notification/0.1/test/test-amp-user-notification.js

@@ -18,6 +18,7 @@ import {
   AmpUserNotification,
   UserNotificationManager,
 } from '../amp-user-notification';
+import {GEO_IN_GROUP} from '../../../amp-geo/0.1/amp-geo';
 import {
   getServiceForDoc,
   getServicePromiseForDoc,
@@ -54,7 +55,9 @@ describes.realWin('amp-user-notification', {
     resetServiceForTesting(win, 'geo');
     registerServiceBuilder(win, 'geo', function() {
       return Promise.resolve({
-        'ISOCountryGroups': ISOCountryGroups,
+        isInCountryGroup: group =>
+          ISOCountryGroups.indexOf(group) >= 0 ?
+            GEO_IN_GROUP.IN : GEO_IN_GROUP.NOT_IN,
       });
     });
 

src/service/url-replacements-impl.js

@@ -330,7 +330,7 @@ export class GlobalVariableSource extends VariableSource {
               'The value passed to AMP_GEO() is not valid name:' + geoType);
           return /** @type {string} */ (geos[geoType] || 'unknown');
         }
-        return /** @type {string} */ (geos.ISOCountryGroups.join(GEO_DELIM));
+        return /** @type {string} */ (geos.matchedISOCountryGroups.join(GEO_DELIM));
       }, 'AMP_GEO');
     }));