♻️📖 amp-geo refactor to remove need for empty config and reduce minified code size #14670
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
approved these changes
Apr 17, 2018
jridgewell
approved these changes
Apr 17, 2018
extensions/amp-geo/0.1/amp-geo.js
Outdated
| @@ -152,24 +153,24 @@ export class AmpGeo extends AMP.BaseElement { | |||
| */ | |||
| matchCountryGroups_(config) { | |||
| /* ISOCountryGroups are optional but if specified at least one must exist */ | |||
| if (config.ISOCountryGroups) { | |||
| const ISOCountryGroups = config.ISOCountryGroups; | |||
| const errorPrefix = '<amp-geo> ISOCountryGroups'; // code size | |||
There was a problem hiding this comment.
Nit: Micro opts like this aren't worth it. The GZIP differences will be minimal, and it just increases code-read burden.
There was a problem hiding this comment.
That one saved 50 bytes.
extensions/amp-geo/0.1/amp-geo.js
Outdated
| this.matchedGroups_.push(groups[i]); | ||
| isArray(ISOCountryGroups[group]), | ||
| `${errorPrefix}[${group}] must be an array`); | ||
| if (ISOCountryGroups[group].indexOf(this.country_) >= 0) { |
There was a problem hiding this comment.
Nit: Array#includes is polyfilled.
|
All tests passed on Travis. The redness in https://travis-ci.org/ampproject/amphtml/jobs/367837186 is coming from Sauce Labs, and is unrelated to the actual tests. Merging this PR. |
rsimha
pushed a commit
that referenced
this pull request
Apr 17, 2018
…ied code size (#14670) * line length, typo * Refactor to reduce minified size by 10%, make jsons config options, doc to match. * touchups * mitigate possible publisher script injection vector * tighten up regex * test for invalid group * indexOf -> array includes
glevitzky
pushed a commit
that referenced
this pull request
Apr 27, 2018
…ied code size (#14670) * line length, typo * Refactor to reduce minified size by 10%, make jsons config options, doc to match. * touchups * mitigate possible publisher script injection vector * tighten up regex * test for invalid group * indexOf -> array includes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Removes the need for an empty
<script>with an empty object in it for default config.Multiple refactors to reduce code size after minification (5853 bytes to -> 5318 bytes 9.1% reduction)
Adjust doc accordingly.