Support --extensions=minimal_set & bug fix
#13249
Conversation
gulpfile.js
Outdated
| @@ -723,8 +742,21 @@ function dist() { | |||
| * Copy built extension to alias extension | |||
| */ | |||
| function copyAliasExtensions() { | |||
| if (!! argv.noextensions) { | |||
gulpfile.js
Outdated
| process.exit(1); | ||
| } | ||
| log(green('Building extension(s):'), | ||
| cyan(argv.extensions.split(',').join(', '))); | ||
| if (argv.extensions == 'minimal_set') { |
There was a problem hiding this comment.
I think you need === here to avoid type conversions.
gulpfile.js
Outdated
| } else { | ||
| log(green('Building extension(s):'), | ||
| cyan(argv.extensions.split(',').join(', '))); | ||
| } |
There was a problem hiding this comment.
Simplify lines 642-651 to:
if (argv.extensions === 'minimal_set') {
argv.extensions =
'amp-ad,amp-ad-network-adsense-impl,amp-audio,amp-video,' +
'amp-image-lightbox,amp-lightbox,amp-sidebar,' +
'amp-analytics,amp-app-banner';
}
log(green('Building extension(s):'),
cyan(argv.extensions.split(',').join(', ')));
gulpfile.js
Outdated
| green('to choose which ones to build.')); | ||
| green('to choose which ones to build, or'), | ||
| cyan('--extensions=minimal_set'), | ||
| green('to build ones needed to load article.amp.html.')); |
There was a problem hiding this comment.
green('to build the ones needed to load'), cyan('article.amp.html') + green('.'));
(added the and color formatting)
gulpfile.js
Outdated
| green('to choose which ones to build.')); | ||
| green('to choose which ones to build, or'), | ||
| cyan('--extensions=minimal_set'), | ||
| green('to build ones needed to load article.amp.html.')); |
There was a problem hiding this comment.
green('to build the ones needed to load'), cyan('article.amp.html') + green('.'));
(added the and color formatting)
gulpfile.js
Outdated
| cyan('--extensions=amp-foo,amp-bar'), | ||
| green('to choose which ones to build, or'), | ||
| cyan('--extensions=minimal_set'), | ||
| green('to build ones needed to load article.amp.html.')); |
There was a problem hiding this comment.
No green in error messages. Use:
log(red('ERROR:'), 'Missing list of extensions. Expected format:',
cyan('--extensions=amp-foo,amp-bar'),
'to choose which ones to build, or',
cyan('--extensions=minimal_set'),
'to build the ones needed to load',
cyan('article.amp.html') + '.');
|
I just realized that by moving all the Could you first address my comments above in one commit so the diffs are easy to review, and then follow it up with one that enables |
|
Nice catch! Got it! |
gulpfile.js
Outdated
| * Prints a helpful message that lets the developer know how to build | ||
| * a list of extensions or without any extensions. | ||
| */ | ||
| function printExtensionsHelp() { |
There was a problem hiding this comment.
This function does more than just print help messages. It also parses the extension flags. Mabye rename it to parseExtensionFlags()?
gulpfile.js
Outdated
| @@ -609,7 +609,7 @@ function printConfigHelp(command) { | |||
| * Prints a helpful message that lets the developer know how to build | |||
There was a problem hiding this comment.
Update function description too :)
* fix dist with alias * change minimal_set usage * fix documentation * refactor print logic * rename * update function description
* fix dist with alias * change minimal_set usage * fix documentation * refactor print logic * rename * update function description
* fix dist with alias * change minimal_set usage * fix documentation * refactor print logic * rename * update function description
Follow up to #13203
@rsimha-amp I didn't update
--extensions=minimal_settoDEVELOPING.mdbecause it was not there before. Also I don't know if we want to document a single special value to the--extensionsflag. Let me know. Thanks!