Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor amp-geo to use externs, #20015

Merged
merged 9 commits into from
Jan 10, 2019
Merged

Refactor amp-geo to use externs, #20015

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
92f7ab7
Refactor amp-geo to use externs,
jpettitt Dec 20, 2018
93ebe09
isInGeoGroup() returns enum.
jpettitt Dec 21, 2018
fb04fb3
bad path
jpettitt Dec 21, 2018
d35c554
merge
jpettitt Jan 10, 2019
4f19fac
revert externs
jpettitt Jan 10, 2019
afe5623
remove stray newline
jpettitt Jan 10, 2019
74b964c
Merge remote-tracking branch 'upstream/master' into amp-geo-refactor
jpettitt Jan 10, 2019
4ea9679
change config to prevent single pass closure mangle
jpettitt Jan 10, 2019
1266238
add new owners
jpettitt Jan 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 21 additions & 0 deletions build-system/amp.extern.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -648,6 +648,27 @@ let BindEvaluateBindingsResultDef;
*/
let BindEvaluateExpressionResultDef;

/////////////////////////////
////// amp-geo externs
/////////////////////////////

/**
* Actual filled values for this exists in
* extensions/amp-geo/0.1/amp-geo.js
* @enum {number}
*/
const GEO_IN_GROUP = {};

/**
* @typedef {{
* ISOCountry: string,
* matchedISOCountryGroups: !Array<string>,
* allISOCountryGroups: !Array<string>,
* isInCountryGroup: (function(string):GEO_IN_GROUP),
* }}
*/
let GeoDef;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you want these fields preserved or are they OK to be mangled during minification?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created the issue because I saw

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

Lines 207 to 208 in 370b6e3

const ISOCountryGroups = /** @type {!Object<string, !Array<string>>} */(
config.ISOCountryGroups);

But since ISOCountryGroups is removed from GeoDef. Is it better to change the above line to config['ISOCountryGroups'] and allow other fields to be mangled instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why and where is it removed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/ampproject/amphtml/pull/20015/files#diff-1771ac83ae00574372906f13b45cdca6L354

Copy link
Contributor Author

@jpettitt jpettitt Dec 21, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erwinmombay Not sure where to go here, suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I couldn't find where GeoDef is directly exposed to 3p. So we should be good to remove it from the extern.

Another thing is we need to change how ampGeo read the configuration. @jpettitt Let me know if you want me to fix that instead. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reverted the externs - what do we need to change on the configuration?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think

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

Lines 215 to 216 in 95fc028

const ISOCountryGroups = /** @type {!Object<string, !Array<string>>} */(
config.ISOCountryGroups);

and

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

Line 339 in 95fc028

if (config.AmpBind) {

need to use config['ISOCountryGroups'] and config['AmpBind'], because we need to preserve the fileNames to read inline config. @erwinmombay to confirm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you talking about the global AMP config?

This config is the am-geo configuration, it comes from parsing the application/json script. If that is the issue it probably makes sense to rename config here to differentiate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked offline. We decided to avoid using dot fieldName to avoid singlepass mangling the value there.


/////////////////////////////
////// Web Anmomation externs
/////////////////////////////
Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-consent/0.1/amp-consent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,7 +452,7 @@ export class AmpConsent extends AMP.BaseElement {
return Services.geoForDocOrNull(this.element).then(geo => {
user().assert(geo,
'requires <amp-geo> to use promptIfUnknownForGeoGroup');
return (geo.ISOCountryGroups.indexOf(geoGroup) >= 0);
return (geo.isInCountryGroup(geoGroup));
jpettitt marked this conversation as resolved.
Show resolved Hide resolved
});
}

Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-consent/0.1/test/test-amp-consent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ describes.realWin('amp-consent', {
resetServiceForTesting(win, 'geo');
registerServiceBuilder(win, 'geo', function() {
return Promise.resolve({
'ISOCountryGroups': ISOCountryGroups,
isInCountryGroup: group => ISOCountryGroups.indexOf(group) >= 0,
});
});

Expand Down
18 changes: 0 additions & 18 deletions extensions/amp-geo/0.1/amp-geo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,17 +91,6 @@ 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>
* }}
*/
export let GeoDef;


export class AmpGeo extends AMP.BaseElement {
/** @param {!AmpElement} element */
Expand Down Expand Up @@ -351,13 +340,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_,
};
});
}
Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-user-notification/0.1/amp-user-notification.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,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);
});

// Invert if includeGeos is false
Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-user-notification/0.1/test/test-amp-user-notification.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ describes.realWin('amp-user-notification', {
resetServiceForTesting(win, 'geo');
registerServiceBuilder(win, 'geo', function() {
return Promise.resolve({
'ISOCountryGroups': ISOCountryGroups,
isInCountryGroup: group => ISOCountryGroups.indexOf(group) >= 0,
});
});

Expand Down
2 changes: 1 addition & 1 deletion src/service/url-replacements-impl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,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');
}));

Expand Down
4 changes: 2 additions & 2 deletions src/services.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,10 +518,10 @@ export class Services {
* Returns a promise for the geo service or a promise for null if
* the service is not available on the current page.
* @param {!Element|!./service/ampdoc-impl.AmpDoc} elementOrAmpDoc
* @return {!Promise<?../extensions/amp-geo/0.1/amp-geo.GeoDef>}
* @return {!Promise<?GeoDef>}
*/
static geoForDocOrNull(elementOrAmpDoc) {
return /** @type {!Promise<?../extensions/amp-geo/0.1/amp-geo.GeoDef>} */ (
return /** @type {!Promise<?GeoDef>} */ (
getElementServiceIfAvailableForDoc(
elementOrAmpDoc, 'geo', 'amp-geo', true));
}
Expand Down