Add --extensions flag to only build listed extensions #13203

zhouyx · 2018-02-01T01:19:25Z

The build time for local testing is getting so long with all these new extensions.
I can't find a way to not build unrelated extensions to my change. Right now I would simply comment out extensions I don't want every time I start the gulp server.

The idea is to introduce a new flag --extension and only build listed extensions.
For example gulp --extension=amp-ad,amp-anim will only build amp-ad and amp-anim extension.
I tested the server only with the gulp default command, please let me know if I miss anything : )

Also I am thinking of the name --only vs --extension. Since we already have --extensions, might be confusing. Let me know, thanks!

rsimha

Thanks for sending this out. This is a great idea!

After looking at the code, I think we should remove the gulp extensions task, and replace it with the --extensions flag to gulp, gulp watch, and gulp build. See my comments below.

rsimha · 2018-02-01T20:00:03Z

gulpfile.js

+        // Skip this extension;
+        continue;
+      }
+    }


Here's a simpler way to rewrite this function:

function buildExtensions(options) { var extensionsToBuild = []; if (!!argv.extensions) { extensionsToBuild = argv.extensions.split(','); } var results = []; for (var key in extensions) { if (extensionsToBuild.length > 0 && extensionsToBuild.indexOf(extensions[key]) == -1) { continue; } var e = extensions[key]; var o = Object.assign({}, options); o = Object.assign(o, e); results.push(buildExtension(e.name, e.version, e.hasCss, o, e.extraGlobs)); } return Promise.all(results); }

For logging, add this code snippet to performBuild() after the call to printConfigHelp():

if (!!argv.extensions) { log(green('Building extension(s):'), cyan(argv.extensions)); }

Also, remove the line...

gulp.task('extensions', 'Build AMP Extensions', buildExtensions);

... from the bottom of the file, and add documentation to gulp watch and gulp build for the new --extensions flag.

It might also be worth adding a --noextensions flag to gulp watch and gulp build that entirely skips building extensions.

Finally, don't forget to update DEVELOPING.md with the new changes.

zhouyx · 2018-02-02T01:02:22Z

@rsimha-amp Thanks for the comments! I've addressed them and added documentation.

rsimha

A few more comments below.

rsimha · 2018-02-02T01:02:28Z

contributing/DEVELOPING.md

@@ -42,11 +42,14 @@ The Quick Start Guide's  [One-time setup](getting-started-quick.md#one-time-setu
 | `gulp lint --watch`                                                     | Watches for changes in files, Validates against Google Closure Linter.|
 | `gulp lint --fix`                                                       | Fixes simple lint warnings/errors automatically.                      |
 | `gulp build`<sup>[[1]](#footnote-1)</sup>                               | Builds the AMP library.                                               |
+| `gulp build --extensions=<extension-name,extension-name>`               | Builds the AMP library with only listed extension components.


nit1: Make the command gulp build --extensions=<amp-foo,amp-bar>
nit2: Make the description Builds the AMP library, with only the listed extensions.

rsimha · 2018-02-02T01:05:35Z

contributing/DEVELOPING.md

@@ -42,11 +42,14 @@ The Quick Start Guide's  [One-time setup](getting-started-quick.md#one-time-setu
 | `gulp lint --watch`                                                     | Watches for changes in files, Validates against Google Closure Linter.|
 | `gulp lint --fix`                                                       | Fixes simple lint warnings/errors automatically.                      |
 | `gulp build`<sup>[[1]](#footnote-1)</sup>                               | Builds the AMP library.                                               |
+| `gulp build --extensions=<extension-name,extension-name>`               | Builds the AMP library with only listed extension components.
+| `gulp build --noextensions`                                             | Builds the AMP library with no extensions.


Add these variations to gulp as well (above).

rsimha · 2018-02-02T01:06:56Z

contributing/DEVELOPING.md

 | `gulp watch`<sup>[[1]](#footnote-1)</sup>                               | Watches for changes in files, re-build.                               |
+| `gulp watch --extensions=<extension-name,extension-name>`               | Watch for changes in files, re-build with only listed extensions.


nit1: Make the command gulp watch --extensions=<amp-foo,amp-bar>
nit2: Make the description Watches for changes in files, re-builds only the listed extensions.

rsimha · 2018-02-02T01:07:55Z

contributing/DEVELOPING.md

 | `gulp watch`<sup>[[1]](#footnote-1)</sup>                               | Watches for changes in files, re-build.                               |
+| `gulp watch --extensions=<extension-name,extension-name>`               | Watch for changes in files, re-build with only listed extensions.
+| `gulp watch --noextensions`                                             | Watch for changes in files, re-build with no extensions.


nit: Watches for changes in files, re-builds with no extensions.

rsimha · 2018-02-02T01:08:19Z

contributing/DEVELOPING.md

 | `gulp check-links --files foo.md,bar.md`                                | Reports dead links in `.md` files.                                                 |
 | `gulp clean`                                                            | Removes build output.                                                 |
 | `gulp css`<sup>[[1]](#footnote-1)</sup>                                 | Recompiles css to build directory and builds the embedded css into js files for the AMP library. |
-| `gulp extensions`                                                       | Build AMP Extensions.                                                 |
 | `gulp watch`<sup>[[1]](#footnote-1)</sup>                               | Watches for changes in files, re-build.                               |


nit: s/re-build/re-builds

rsimha · 2018-02-02T01:10:09Z

gulpfile.js

@@ -241,8 +241,25 @@ function endBuildStep(stepName, targetName, startTime) {
 * @return {!Promise}
 */
 function buildExtensions(options) {
-  var results = [];
+  const results = [];


This shouldn't be a const, since you're editing it below. Revert this line.

rsimha · 2018-02-02T01:10:28Z

gulpfile.js

+    return Promise.all(results);
+  }
+
+  // --extensions flag: only build listed extensions


The comment is unnecessary, remove.

rsimha · 2018-02-02T01:12:37Z

gulpfile.js

+  let extensionsToBuild = [];
+  // --noextensions flag skip building extensions
+  if (!!argv.noextensions) {
+    return Promise.all(results);


Move this to the top and return Promise.resolve();

rsimha · 2018-02-02T01:14:38Z

gulpfile.js

+        // Skip this extension;
+        continue;
+      }
+    }


Here's a cleaner version of this function with your changes:

function buildExtensions(options) { if (!!argv.noextensions) { return Promise.resolve(); } var extensionsToBuild = []; if (!!argv.extensions) { extensionsToBuild = argv.extensions.split(','); } var results = []; for (var key in extensions) { if (extensionsToBuild.length > 0 && extensionsToBuild.indexOf(extensions[key]) == -1) { continue; } var e = extensions[key]; var o = Object.assign({}, options); o = Object.assign(o, e); results.push(buildExtension(e.name, e.version, e.hasCss, o, e.extraGlobs)); } return Promise.all(results); }

rsimha · 2018-02-02T01:16:01Z

gulpfile.js

@@ -1349,7 +1369,6 @@ gulp.task('dist', 'Build production binaries', ['update-packages'], dist, {
    minimal_set: '  Only compile files needed to load article.amp.html',
  }
 });
-gulp.task('extensions', 'Build AMP Extensions', buildExtensions);
 gulp.task('watch', 'Watches for changes in files, re-builds when detected',
    ['update-packages'], watch, {
      options: {


Add documentation for --extensions and --noextensions here, and for gulp and gulp build.

rsimha

Yay! LGTM after these documentation fixes.

This is very cool stuff. The build time just went down from a couple minutes to a few seconds. Everyone is going to be so happy when you announce this :)

rsimha · 2018-02-02T01:54:37Z

contributing/DEVELOPING.md

@@ -35,21 +35,23 @@ The Quick Start Guide's  [One-time setup](getting-started-quick.md#one-time-setu
 | Command                                                                 | Description                                                           |
 | ----------------------------------------------------------------------- | --------------------------------------------------------------------- |
 | **`gulp`**<sup>[[1]](#footnote-1)</sup>                                 | Runs "watch" and "serve". Use this for standard local dev.            |
+| `gulp --extensions=<amp-foo,amp-bar>`                                   | Runs "watch" and "serve" with only listed extension components.


nit: Runs "watch" and "serve", after building only the listed extensions.

rsimha · 2018-02-02T01:54:58Z

contributing/DEVELOPING.md

@@ -35,21 +35,23 @@ The Quick Start Guide's  [One-time setup](getting-started-quick.md#one-time-setu
 | Command                                                                 | Description                                                           |
 | ----------------------------------------------------------------------- | --------------------------------------------------------------------- |
 | **`gulp`**<sup>[[1]](#footnote-1)</sup>                                 | Runs "watch" and "serve". Use this for standard local dev.            |
+| `gulp --extensions=<amp-foo,amp-bar>`                                   | Runs "watch" and "serve" with only listed extension components.
+| `gulp --noextensions`                                                   | Runs "watch" and "serve" with no extensions.


nit: Runs "watch" and "serve", without building any extensions.

rsimha · 2018-02-02T01:55:54Z

contributing/DEVELOPING.md

 | `gulp dist`<sup>[[1]](#footnote-1)</sup>                                | Builds production binaries.                                           |
 | `gulp dist --fortesting`<sup>[[1]](#footnote-1)</sup>                   | Builds production binaries for local testing. (Allows use cases like ads, tweets, etc. to work with minified sources. Overrides `TESTING_HOST` if specified. Uses the production `AMP_CONFIG` by default.) |
 | `gulp dist --fortesting --config=<config>`<sup>[[1]](#footnote-1)</sup> | Builds production binaries for local testing, with the specified `AMP_CONFIG`. `config` can be `prod` or `canary`. (Defaults to `prod`.) |
 | `gulp lint`                                                             | Validates against Google Closure Linter.                              |
 | `gulp lint --watch`                                                     | Watches for changes in files, Validates against Google Closure Linter.|
 | `gulp lint --fix`                                                       | Fixes simple lint warnings/errors automatically.                      |
 | `gulp build`<sup>[[1]](#footnote-1)</sup>                               | Builds the AMP library.                                               |
+| `gulp build --extensions=<amp-foo,amp-bar>`                         | Builds the AMP library, with only the listed extensions.


nit: For easy editing, add spaces to align the | with the line above.

rsimha · 2018-02-02T01:56:45Z

gulpfile.js

@@ -1332,14 +1348,21 @@ function toPromise(readable) {
 gulp.task('build', 'Builds the AMP library', ['update-packages'], build, {
  options: {
    config: '  Sets the runtime\'s AMP_CONFIG to one of "prod" or "canary"',
+    extensions: '  Builds only the listed extensions.',
+    noextensions: ' Builds with no extensions.',


Two spaces between ' and Builds

rsimha · 2018-02-02T02:07:38Z

Update the PR title to indicate the new name of the flag --extensions.

* add --extension flag to only build some extensions * address comment * fix * nit

zhouyx requested review from erwinmombay and rsimha February 1, 2018 01:19

googlebot added the cla: yes label Feb 1, 2018

zhouyx changed the title ~~add --extension flag to only build some extensions~~ Enable --extension flag to only build some extensions Feb 1, 2018

rsimha suggested changes Feb 1, 2018

View reviewed changes

zhouyx force-pushed the gulp-with-extensions branch from c56be70 to eec17c8 Compare February 2, 2018 01:01

rsimha suggested changes Feb 2, 2018

View reviewed changes

rsimha approved these changes Feb 2, 2018

View reviewed changes

zhouyx changed the title ~~Enable --extension flag to only build some extensions~~ Add --extensions flag to only build listed extensions Feb 2, 2018

zhouyx force-pushed the gulp-with-extensions branch from 8090022 to f50c835 Compare February 2, 2018 19:03

zhouyx added 4 commits February 2, 2018 13:14

add --extension flag to only build some extensions

48026c9

address comment

3777c47

fix

17a76ff

nit

c32acc7

zhouyx force-pushed the gulp-with-extensions branch from f50c835 to c32acc7 Compare February 2, 2018 21:19

zhouyx merged commit 60c7428 into ampproject:master Feb 2, 2018

rsimha mentioned this pull request Feb 2, 2018

Document usage of --noextensions and --extensions #13246

Merged

zhouyx mentioned this pull request Feb 2, 2018

Support --extensions=minimal_set & bug fix #13249

Merged

ampprojectbot added the PR use: In Stable label Feb 14, 2018

protonate pushed a commit to protonate/amphtml that referenced this pull request Feb 26, 2018

Add --extensions flag to only build listed extensions (ampproject#13203)

c8ec0d3

* add --extension flag to only build some extensions * address comment * fix * nit

RanAbram pushed a commit to RanAbram/amphtml that referenced this pull request Mar 12, 2018

Add --extensions flag to only build listed extensions (ampproject#13203)

33f2e4b

* add --extension flag to only build some extensions * address comment * fix * nit

protonate pushed a commit to protonate/amphtml that referenced this pull request Mar 15, 2018

Add --extensions flag to only build listed extensions (ampproject#13203)

cf3cfb2

* add --extension flag to only build some extensions * address comment * fix * nit

rsimha mentioned this pull request Jun 3, 2019

🏗♻️ Refactor / clean up the watch logic used for extensions #22659

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --extensions flag to only build listed extensions #13203

Add --extensions flag to only build listed extensions #13203

zhouyx commented Feb 1, 2018

rsimha left a comment

rsimha Feb 1, 2018 •

edited

rsimha Feb 1, 2018

rsimha Feb 1, 2018 •

edited

rsimha Feb 1, 2018

rsimha Feb 1, 2018

zhouyx commented Feb 2, 2018

rsimha left a comment

rsimha Feb 2, 2018

zhouyx Feb 2, 2018

rsimha Feb 2, 2018

zhouyx Feb 2, 2018

rsimha Feb 2, 2018

zhouyx Feb 2, 2018

rsimha Feb 2, 2018

zhouyx Feb 2, 2018

rsimha Feb 2, 2018

zhouyx Feb 2, 2018

rsimha Feb 2, 2018

zhouyx Feb 2, 2018

rsimha Feb 2, 2018

zhouyx Feb 2, 2018

rsimha Feb 2, 2018

zhouyx Feb 2, 2018

rsimha Feb 2, 2018

rsimha Feb 2, 2018

zhouyx Feb 2, 2018

rsimha left a comment

rsimha Feb 2, 2018

rsimha Feb 2, 2018

rsimha Feb 2, 2018

rsimha Feb 2, 2018

rsimha commented Feb 2, 2018

		\| `gulp watch`<sup>[[1]](#footnote-1)</sup> \| Watches for changes in files, re-build. \|
		\| `gulp watch --extensions=<extension-name,extension-name>` \| Watch for changes in files, re-build with only listed extensions.

Add --extensions flag to only build listed extensions #13203

Add --extensions flag to only build listed extensions #13203

Conversation

zhouyx commented Feb 1, 2018

rsimha left a comment

Choose a reason for hiding this comment

rsimha Feb 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsimha Feb 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhouyx commented Feb 2, 2018

rsimha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsimha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsimha commented Feb 2, 2018

rsimha Feb 1, 2018 •

edited

rsimha Feb 1, 2018 •

edited