Gulp serve only serve either compiled or uncompiled version #8559

zhouyx · 2017-04-03T20:31:17Z

For #7581
Please let me know if the approach looks good in general.
Will get to integration test change in different PR. Also it would require some doc change.
Heroku would by default run max version. Can someone point to me where the config is for heroku?

zhouyx · 2017-04-03T20:32:59Z

src/service/extension-location.js

-export function calculateEntryPointScriptUrl(location, entryPoint, isLocalDev,
-    isTest) {
-  const base = calculateScriptBaseUrl(location, isLocalDev, isTest);
-  const serveMax = isLocalDev && isMax(location);


@choumx @jridgewell any ideas why we don't take isTest into consideration here?

Probably just an oversight (or deemed unnecessary). We'd also need isUsingCompiledJs to make sure we test min builds with --compiled here.

good news is with this PR, gulp server will use min files with --compiled flag. we don't need to deal with isUsingCompiledJs anymore.

cramforce · 2017-04-04T02:20:44Z

build-system/tasks/serve.js

@@ -23,6 +23,18 @@ var nodemon = require('nodemon');
 * Starts a simple http server at the repository root
 */
 function serve() {
+  // Get the serve mode
+  if (argv.compiled) {
+    process.env.SERVE_MODE = 'min';


Could we make this changeable at runtime?

I can add the check for app.use('/serve_mode=?') to change it at runtime. If this is what you mean?

cramforce · 2017-04-04T17:36:01Z

Yep.

…

On Tue, Apr 4, 2017 at 10:33 AM, Yuxuan Zhou ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In build-system/tasks/serve.js <#8559 (comment)>: > @@ -23,6 +23,18 @@ var nodemon = require('nodemon'); * Starts a simple http server at the repository root */ function serve() { + // Get the serve mode + if (argv.compiled) { + process.env.SERVE_MODE = 'min'; I can add the check for app.use('/serve_mode=?') to change it at runtime. If this is what you mean? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#8559 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFeT9YiXgAnBk3ph9zgcfPHEC4P_AF3ks5rsn7ogaJpZM4MyDcK> .

zhouyx · 2017-04-04T23:10:17Z

PTAL 👀

zhouyx · 2017-04-06T23:42:09Z

Ping~

zhouyx · 2017-04-11T22:09:21Z

Friendly ping on this PR. @cramforce Do we still want to keep it? : )

cramforce · 2017-04-11T22:41:45Z

I'm just waiting for others to review :)

zhouyx · 2017-04-11T22:54:48Z

LOLOL 😄 Ping @erwinmombay @lannka and @dvoytenko

dvoytenko · 2017-04-12T17:52:30Z

build-system/app.js

@@ -28,9 +28,31 @@ var jsdom = require('jsdom');
 var path = require('path');
 var request = require('request');
 var url = require('url');
+var mode = process.env.SERVE_MODE;


Can you document here what's default is?

dvoytenko · 2017-04-12T17:52:42Z

build-system/app.js

+    process.env.SERVE_MODE = newMode;
+    info = '<h2>Serve mode changed to ' + newMode + '</h2>';
+  } else {
+    info = '<h2>Serve mode ' + newMode + ' is not support. </h2>';


dvoytenko · 2017-04-12T17:54:35Z

build-system/app.js

@@ -486,19 +511,31 @@ app.use('/impression-proxy/', function(req, res) {
 // Example:
 // http://localhost:8000/max/s/www.washingtonpost.com/amphtml/news/post-politics/wp/2016/02/21/bernie-sanders-says-lower-turnout-contributed-to-his-nevada-loss-to-hillary-clinton/
 app.use('/max/', function(req, res) {
+  if (mode != 'max') {


How about we just rename it /proxy/ or /cdn/? Then, we'll just need one and we can remove /min/

dvoytenko · 2017-04-12T17:58:31Z

src/service/extension-location.js

-    if (isTest || isMax(location) || isMin(location)) {
-      return `${location.protocol}//${location.host}/dist`;
-    }
+    return `${location.protocol}//${location.host}/dist`;


I really dislike isLocalDev being passed here. At the minimum, it kills the DCE. I suppose we can live with it, at least for this PR. But if it's easy to remove, let's do.

I don't know a good way to remove isLocalDev here 😢

Agree, DCE of localDev code can be a separate project :-). I do see many code can be DCEed if we simplify our isLocalDev logic here

dvoytenko · 2017-04-12T17:59:29Z

src/service/extension-location.js

-  return path.indexOf('.min') >= 0 || path.substr(0, 5) == '/min/';
+export function calculateEntryPointScriptUrl(location, entryPoint, isLocalDev) {
+  const base = calculateScriptBaseUrl(location, isLocalDev);
+  // QQQ: why we don't use .max version for test here?


confirmed. removed.

zhouyx · 2017-04-13T18:01:43Z

addressed comments from @dvoytenko. PTAL
@jridgewell would you take a look at the change related to service worker once you're available.
@bpaduch will need you to review the change on developing.md.
Thanks a lot!

ghost · 2017-04-18T20:22:40Z

contributing/DEVELOPING.md

+### Serve Mode
+There are 3 serving modes:
+- MAX mode serves unminified AMP. You want to use this during normal dev. `gulp` serves MAX mode by default.
+- MIN mode serves minified AMP. This is closer to the prod setup. This is only avaialable after running `gulp dist --fortesting`. Serve MIN mode by adding `--compiled` to `gulp` command.


correct spelling of "available"

ghost · 2017-04-18T20:23:38Z

contributing/DEVELOPING.md

+There are 3 serving modes:
+- MAX mode serves unminified AMP. You want to use this during normal dev. `gulp` serves MAX mode by default.
+- MIN mode serves minified AMP. This is closer to the prod setup. This is only avaialable after running `gulp dist --fortesting`. Serve MIN mode by adding `--compiled` to `gulp` command.
+- CDN mode serves prod. This file would not reflect your local changes. Serve CDN mode by adding `--cdn` to `gulp` command.


"this file" - what file? Please clarify

ghost · 2017-04-18T20:25:33Z

contributing/DEVELOPING.md


-For each example there are 3 modes:
+To switch serving mode in runtime. please to to http://localhost:8000/serve_mode=$mode. $mode can be `max/min/cdn`.


Rewrite as:

To switch serving modes during runtime, go to http://localhost:8000/serve_mode=$mode and set the $mode to one of the following values: max, min, or cdn.

ghost · 2017-04-18T20:28:41Z

contributing/DEVELOPING.md

@@ -84,15 +85,14 @@ For any public AMP document like: http://output.jsbin.com/pegizoq/quiet

 You can access is with the local JS at

- normal sources: http://localhost:8000/max/output.jsbin.com/pegizoq/quiet
- minified: http://localhost:8000/min/output.jsbin.com/pegizoq/quiet
+http://localhost:8000/proxy/output.jsbin.com/pegizoq/quiet


Correct wording of this section to:

For any public AMP document like http://output.jsbin.com/pegizoq/quiet, you can access it with the local JS at http://localhost:8000/proxy/output.jsbin.com/pegizoq/quiet.

ghost · 2017-04-18T20:30:11Z

contributing/DEVELOPING.md


-When accessing `min` urls make sure you run `gulp dist` with the `--fortesting`
+Note local proxy will serve minified or unminified JS based on current serve mode. When serve mode is `cdn` local proxy will serve remote JS.


Rewrite as follows:

**Note**: The local proxy will serve minified or unminified JS based on the current serve mode. When the serve mode is cdn, the local proxy will serve remote JS.

zhouyx · 2017-04-21T17:48:43Z

modified doc.
Ping @jridgewell to take a look. 😄

lannka · 2017-04-24T23:45:34Z

build-system/app.js

+});
+
+// Deprecate usage of .min.html/.max.html
+app.use(['/examples/*.(min|max).html', '/test/manual/*.(min|max).html',


lannka · 2017-04-24T23:47:05Z

build-system/app.js


 app.use(bodyParser.json());

+app.use('/serve_mode=:mode', function (req, res, next) {


lannka · 2017-04-24T23:51:13Z

build-system/app.js

+    process.env.SERVE_MODE = newMode;
+    info = '<h2>Serve mode changed to ' + newMode + '</h2>';
+  } else {
+    info = '<h2>Serve mode ' + newMode + ' is not supported. </h2>';


nit: res.status(400).send(info)

lannka · 2017-04-24T23:54:08Z

build-system/tasks/serve.js

+    process.env.SERVE_MODE = 'cdn';
+    util.log(util.colors.green('Serving current prod js'));
+  } else {
+    process.env.SERVE_MODE = 'max';


nit: to be consistent with the flags, can we name them: compiled|cdn|default?

But I think we are more familiar with min/max. And it's not obvious right now that default is max mode.

But I thought this whole PR is to get rid of min/max concept, and only stay with the "compiled" concept.

lannka · 2017-04-24T23:56:12Z

build-system/app.js

@@ -28,9 +28,32 @@ var jsdom = require('jsdom');
 var path = require('path');
 var request = require('request');
 var url = require('url');
+// SERVE_MODE is defaulted to be 'max' if not specified
+var mode = process.env.SERVE_MODE;


how about just MODE?

I like SERVE_MODE better because we already have SERVE_PORT SERVE_HOST...

jridgewell · 2017-04-25T19:07:12Z

build-system/app.js

+        // Require url from cdn
+        var filePath = 'https://cdn.ampproject.org/' + fileName;
+        request(filePath, function(error, response) {
+          res.send(response);


Check for the error.

jridgewell · 2017-04-25T19:07:22Z

build-system/app.js

+    // This will not be useful until extension-location.js change in prod
+    // Require url from cdn
+    request(filePath, function (error, response) {
+      res.send(response);


Check for the error.

jridgewell · 2017-04-25T19:08:28Z

build-system/app.js

+      }
+      if (mode == 'max') {
+        var fileUrl = req.url;
+        req.url = fileUrl.substr(0, fileUrl.length - 3) + '.max.js';


Nit: req.url = req.url.replace(/\.js$/, '.max.js')

lannka

could you also change inabox.host.html, to remove the max. in iframe src.

zhouyx · 2017-04-28T22:40:25Z

Done. Will finally merge this after travis pass 🤓

muxin · 2017-04-28T23:47:24Z

test/manual/accessibility/aria-propagation.amp.html

-    <script async custom-element="amp-video" src="../../../dist/v0/amp-video-0.1.max.js"></script>
+    <script async custom-element="amp-anim" src="https://cdn.ampproject.org/v0/amp-anim-0.1.js"></script>
+    <script async custom-element="amp-audio" src="https://cdn.ampproject.org/v0/amp-audio-0.1.js"></script>
+    <script async custom-element="amp-video" src="https://cdn.ampproject.org//amp-video-0.1.js"></script>


Missing v0 here?

…ct#8559)

zhouyx requested review from cramforce, erwinmombay and lannka April 3, 2017 20:31

ampwafflebot added the in progress label Apr 3, 2017

zhouyx commented Apr 3, 2017

View reviewed changes

cramforce reviewed Apr 4, 2017

View reviewed changes

zhouyx force-pushed the max-min branch from 82cacfd to 8bc3943 Compare April 4, 2017 23:06

zhouyx force-pushed the max-min branch from 1a348f0 to 201aa48 Compare April 4, 2017 23:51

zhouyx requested review from dvoytenko and removed request for lannka April 10, 2017 17:06

dvoytenko suggested changes Apr 12, 2017

View reviewed changes

zhouyx force-pushed the max-min branch from 3252da0 to 43f1896 Compare April 13, 2017 00:33

ghost suggested changes Apr 18, 2017

View reviewed changes

ghost approved these changes Apr 21, 2017

View reviewed changes

lannka approved these changes Apr 24, 2017

View reviewed changes

zhouyx force-pushed the max-min branch from 4002a2d to 6ebe494 Compare April 25, 2017 18:06

jridgewell approved these changes Apr 25, 2017

View reviewed changes

lannka approved these changes Apr 26, 2017

View reviewed changes

dvoytenko approved these changes Apr 26, 2017

View reviewed changes

lannka reviewed Apr 28, 2017

View reviewed changes

Deprecate usage of .min.html & .max.html

46ac693

zhouyx added 13 commits April 28, 2017 15:44

server replace url with versioned

e018413

support cdn

92c3176

fix manual test url

bc14b1c

add serve_mode=:mode

a1941fa

change doc

56927b7

remove /max/ /min

1ebd3ad

remove note

9adbfd2

use /proxy for a4a inabox

ba9986a

change doc

2cc48e6

address comment

3589e3f

check for error

bf0fd54

change name to compiled/default/cdn

73cbf01

one last fix

e9acf84

zhouyx force-pushed the max-min branch from 966e24e to e9acf84 Compare April 28, 2017 22:44

zhouyx merged commit eb802dc into ampproject:master Apr 28, 2017

muxin reviewed Apr 28, 2017

View reviewed changes

This was referenced May 1, 2017

Weekly Status 2017-05-01 #9058

Closed

Only serve either max or min mode with one gulp server #7581

Closed

dreamofabear mentioned this pull request May 1, 2017

Express.js serves images as text/html #9070

Closed

KenneyE pushed a commit to spotxchange/amphtml that referenced this pull request May 3, 2017

Gulp serve only serve either compiled or uncompiled version (ampproje…

9fadbc6

…ct#8559)

ampprojectbot added PR use: In Beta / Experimental PR use: In Stable labels May 11, 2017

muxin mentioned this pull request Jun 13, 2017

insertExtensionScriptIfNeeded generates wrong url in heroku #9873

Closed

zhouyx mentioned this pull request Jun 14, 2017

Remove all .max usage in doc #9931

Closed

lannka mentioned this pull request Jul 14, 2017

Introduce iframe transport to amp-analytics #10442

Merged


		For each example there are 3 modes:
		To switch serving mode in runtime. please to to http://localhost:8000/serve_mode=$mode. $mode can be `max/min/cdn`.


		When accessing `min` urls make sure you run `gulp dist` with the `--fortesting`
		Note local proxy will serve minified or unminified JS based on current serve mode. When serve mode is `cdn` local proxy will serve remote JS.


		app.use(bodyParser.json());

		app.use('/serve_mode=:mode', function (req, res, next) {

Gulp serve only serve either compiled or uncompiled version #8559

Gulp serve only serve either compiled or uncompiled version #8559

Conversation

zhouyx commented Apr 3, 2017

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

cramforce commented Apr 4, 2017 via email

zhouyx commented Apr 4, 2017

zhouyx commented Apr 6, 2017

zhouyx commented Apr 11, 2017

cramforce commented Apr 11, 2017

zhouyx commented Apr 11, 2017

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

zhouyx commented Apr 13, 2017

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

zhouyx commented Apr 21, 2017

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

lannka left a comment

Choose a reason for hiding this comment

zhouyx commented Apr 28, 2017

Choose a reason for hiding this comment