Refactor amp-geo to use externs, #20015
Conversation
update dependant extension to use isInCountryGroup()
build-system/amp.extern.js
Outdated
| * isInCountryGroup: (function(string):GEO_IN_GROUP), | ||
| * }} | ||
| */ | ||
| let GeoDef; |
There was a problem hiding this comment.
do you want these fields preserved or are they OK to be mangled during minification?
There was a problem hiding this comment.
I created the issue because I saw
amphtml/extensions/amp-geo/0.1/amp-geo.js
Lines 207 to 208 in 370b6e3
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?
There was a problem hiding this comment.
why and where is it removed?
There was a problem hiding this comment.
There was a problem hiding this comment.
@erwinmombay Not sure where to go here, suggestions?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I've reverted the externs - what do we need to change on the configuration?
There was a problem hiding this comment.
I think
amphtml/extensions/amp-geo/0.1/amp-geo.js
Lines 215 to 216 in 95fc028
and
amphtml/extensions/amp-geo/0.1/amp-geo.js
Line 339 in 95fc028
need to use
config['ISOCountryGroups'] and config['AmpBind'], because we need to preserve the fileNames to read inline config. @erwinmombay to confirm.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Talked offline. We decided to avoid using dot fieldName to avoid singlepass mangling the value there.
* 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
Refactor amp-geo to use externs, update dependant extension to use isInCountryGroup()
Fixes #19968