Allow user to specify mode for pub build/serve.

Allow user to specify mode for pub build/serve.

BUG=https://code.google.com/p/dart/issues/detail?id=14792
R=nweiz@google.com

Committed: https://code.google.com/p/dart/source/detail?r=30262

[nweiz, 2013-11-12 01:49:53]

https://codereview.chromium.org/69043003/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/command/build.dart (right):

https://codereview.chromium.org/69043003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/build.dart:45: help: 'Minify generated
JavaScript.');
This would be a good opportunity to subsume --minify into --mode.

https://codereview.chromium.org/69043003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/build.dart:90: assets =
assets.where((asset) => asset.id.extension != ".dart");
If this is being tied to the mode, ".dart" files shouldn't be emitted when
running "pub serve" in release mode.

One possibility would be to make DartForwardingTransformer know the mode and use
it to decide whether to forward or consume all Dart files.

[Bob Nystrom, 2013-11-12 20:50:09]

https://codereview.chromium.org/69043003/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/command/build.dart (right):

https://codereview.chromium.org/69043003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/build.dart:45: help: 'Minify generated
JavaScript.');
On 2013/11/12 01:49:54, nweiz wrote:
> This would be a good opportunity to subsume --minify into --mode.

I think --minify should actually be configured in the pubspec something like:

transformers:
- dart2js:
    minify: true

Until we have actual configuration support for the dart2js transformer, though,
I'd rather just leave this as-is. It doesn't really hurt to have a separate flag
for it.

https://codereview.chromium.org/69043003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/build.dart:90: assets =
assets.where((asset) => asset.id.extension != ".dart");
On 2013/11/12 01:49:54, nweiz wrote:
> If this is being tied to the mode, ".dart" files shouldn't be emitted when
> running "pub serve" in release mode.
> 
> One possibility would be to make DartForwardingTransformer know the mode and
use
> it to decide whether to forward or consume all Dart files.

Done.

[nweiz, 2013-11-12 21:11:50]

https://codereview.chromium.org/69043003/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/command/build.dart (right):

https://codereview.chromium.org/69043003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/build.dart:45: help: 'Minify generated
JavaScript.');
On 2013/11/12 20:50:09, Bob Nystrom wrote:
> On 2013/11/12 01:49:54, nweiz wrote:
> > This would be a good opportunity to subsume --minify into --mode.
> 
> I think --minify should actually be configured in the pubspec something like:
> 
> transformers:
> - dart2js:
>     minify: true

Sure; any transformer's functionality whose default is based on the mode should
also be explicitly configurable. But the default should still be based on the
mode.

> Until we have actual configuration support for the dart2js transformer,
though,
> I'd rather just leave this as-is. It doesn't really hurt to have a separate
flag
> for it.

I disagree. Every other piece of functionality that differs between "pub serve"
and "pub deploy" is tied to mode -- users will assume, reasonably, that
minification is as well. It will cause them frustration and confusion when
"--mode=release" isn't enough to simulate "pub deploy" on the server, or when
"--mode=debug" isn't enough to simulate "pub serve" in the build.

This is basically the same issue as when the VM had a bunch of different flags
for different aspects of checked mode without a unified "--checked" flag.

https://codereview.chromium.org/69043003/diff/190001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/barback/dart_forwarding_transformer.dart
(right):

https://codereview.chromium.org/69043003/diff/190001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/dart_forwarding_transformer.dart:14: ///
not in release mode.
Explain that in release mode, it stops any unconsumed ".dart" files from being
forwarded.

[Bob Nystrom, 2013-11-12 21:28:06]

https://codereview.chromium.org/69043003/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/command/build.dart (right):

https://codereview.chromium.org/69043003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/build.dart:45: help: 'Minify generated
JavaScript.');
On 2013/11/12 21:11:50, nweiz wrote:
> On 2013/11/12 20:50:09, Bob Nystrom wrote:
> > On 2013/11/12 01:49:54, nweiz wrote:
> > > This would be a good opportunity to subsume --minify into --mode.
> > 
> > I think --minify should actually be configured in the pubspec something
like:
> > 
> > transformers:
> > - dart2js:
> >     minify: true
> 
> Sure; any transformer's functionality whose default is based on the mode
should
> also be explicitly configurable. But the default should still be based on the
> mode.

Right now, it's based on the command, which is a rough approximation of the
mode.

> 
> > Until we have actual configuration support for the dart2js transformer,
> though,
> > I'd rather just leave this as-is. It doesn't really hurt to have a separate
> flag
> > for it.
> 
> I disagree. Every other piece of functionality that differs between "pub
serve"
> and "pub deploy" is tied to mode -- users will assume, reasonably, that
> minification is as well. It will cause them frustration and confusion when
> "--mode=release" isn't enough to simulate "pub deploy" on the server, or when
> "--mode=debug" isn't enough to simulate "pub serve" in the build.
> 
> This is basically the same issue as when the VM had a bunch of different flags
> for different aspects of checked mode without a unified "--checked" flag.

Right, but we don't have a bunch of flags. We have two, one of which is already
submitted, tested, and documented.

I don't see the value in removing that code and documentation and losing
functionality in order to simplify something that's already not complex.

https://codereview.chromium.org/69043003/diff/190001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/barback/dart_forwarding_transformer.dart
(right):

https://codereview.chromium.org/69043003/diff/190001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/dart_forwarding_transformer.dart:14: ///
not in release mode.
On 2013/11/12 21:11:51, nweiz wrote:
> Explain that in release mode, it stops any unconsumed ".dart" files from being
> forwarded.

Done.

[nweiz, 2013-11-12 22:16:20]

https://codereview.chromium.org/69043003/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/command/build.dart (right):

https://codereview.chromium.org/69043003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/build.dart:45: help: 'Minify generated
JavaScript.');
On 2013/11/12 21:28:06, Bob Nystrom wrote:
> Right now, it's based on the command, which is a rough approximation of the
> mode.

Why have a rough approximation when you could have the actual thing?

> Right, but we don't have a bunch of flags. We have two, one of which is
already
> submitted, tested, and documented.

I don't understand your point here. Are you saying that because one flag was
already created, keeping them split provides better UI? That seems ridiculous to
me. The fact that code exists isn't in and of itself justification for keeping
it around.

"--minify" was created before "--mode" existed. If we had written "--mode"
first, there's no way we would have made "--minify" separate.

> I don't see the value in removing that code and documentation and losing
> functionality in order to simplify something that's already not complex.

Pub+barback is *incredibly* complex, and this is part of that system. Most of
that complexity is necessary to support all the use cases we need to, but it's
our job to eliminate any unnecessary complexity, wherever it exists. This is
unnecessary complexity.

As for the value, I've already explained it, but here it is again: users won't
understand the distinction between "pub serve mode vs pub build mode" and "debug
mode vs release mode". They'll try to run "pub serve --mode=release" and expect
it to minify their code. It won't, and they'll be frustrated and unhappy.

That's a use case where splitting "--minify" and "--mode" will cause actual
users actual pain. Is there a use-case where splitting them benefits users that
won't be handled by letting users refer to the dart2js transformer in the
pubspec?

[Bob Nystrom, 2013-11-12 22:25:46]

On 2013/11/12 22:16:20, nweiz wrote:
>
https://codereview.chromium.org/69043003/diff/1/sdk/lib/_internal/pub/lib/src...
> File sdk/lib/_internal/pub/lib/src/command/build.dart (right):
> 
>
https://codereview.chromium.org/69043003/diff/1/sdk/lib/_internal/pub/lib/src...
> sdk/lib/_internal/pub/lib/src/command/build.dart:45: help: 'Minify generated
> JavaScript.');
> On 2013/11/12 21:28:06, Bob Nystrom wrote:
> > Right now, it's based on the command, which is a rough approximation of the
> > mode.
> 
> Why have a rough approximation when you could have the actual thing?
> 
> > Right, but we don't have a bunch of flags. We have two, one of which is
> already
> > submitted, tested, and documented.
> 
> I don't understand your point here. Are you saying that because one flag was
> already created, keeping them split provides better UI? That seems ridiculous
to
> me. The fact that code exists isn't in and of itself justification for keeping
> it around.

What I'm saying is that the current behavior lets you specify things that
removing the flag does not.

> 
> "--minify" was created before "--mode" existed. If we had written "--mode"
> first, there's no way we would have made "--minify" separate.
> 
> > I don't see the value in removing that code and documentation and losing
> > functionality in order to simplify something that's already not complex.
> 
> Pub+barback is *incredibly* complex, and this is part of that system. Most of
> that complexity is necessary to support all the use cases we need to, but it's
> our job to eliminate any unnecessary complexity, wherever it exists. This is
> unnecessary complexity.

Yes, and are you trying to argue that removing a flag (including removing its
documentation) is the best use of my time for eliminating that complexity?

> 
> As for the value, I've already explained it, but here it is again: users won't
> understand the distinction between "pub serve mode vs pub build mode" and
"debug
> mode vs release mode". They'll try to run "pub serve --mode=release" and
expect
> it to minify their code. It won't, and they'll be frustrated and unhappy.

I don't know if users will use "pub serve --mode=release" much at all. I only
added it because I was adding support for --mode to pub build so that users
could do a build that included Dart files. Adding it to pub serve just seemed
like a symmetric change.

> That's a use case where splitting "--minify" and "--mode" will cause actual
> users actual pain. Is there a use-case where splitting them benefits users
that
> won't be handled by letting users refer to the dart2js transformer in the
> pubspec?

I don't really know what the use cases are for --minify. We had a concrete use
case where users wanted pub serve to not minify, and I added the flag at that
time because it seemed useful to be able to configure it. We don't have the
feedback to tell how useful it is.

I don't get why you're digging your heels in here. --minify is *already* in the
codebase. Why is this patch obligated to remove it?

[nweiz, 2013-11-12 23:04:59]

On 2013/11/12 22:25:46, Bob Nystrom wrote:
> On 2013/11/12 22:16:20, nweiz wrote:
> >
>
https://codereview.chromium.org/69043003/diff/1/sdk/lib/_internal/pub/lib/src...
> > File sdk/lib/_internal/pub/lib/src/command/build.dart (right):
> > 
> >
>
https://codereview.chromium.org/69043003/diff/1/sdk/lib/_internal/pub/lib/src...
> > sdk/lib/_internal/pub/lib/src/command/build.dart:45: help: 'Minify generated
> > JavaScript.');
> > On 2013/11/12 21:28:06, Bob Nystrom wrote:
> > > Right now, it's based on the command, which is a rough approximation of
the
> > > mode.
> > 
> > Why have a rough approximation when you could have the actual thing?
> > 
> > > Right, but we don't have a bunch of flags. We have two, one of which is
> > already
> > > submitted, tested, and documented.
> > 
> > I don't understand your point here. Are you saying that because one flag was
> > already created, keeping them split provides better UI? That seems
ridiculous
> to
> > me. The fact that code exists isn't in and of itself justification for
keeping
> > it around.
> 
> What I'm saying is that the current behavior lets you specify things that
> removing the flag does not.
> 
> > 
> > "--minify" was created before "--mode" existed. If we had written "--mode"
> > first, there's no way we would have made "--minify" separate.
> > 
> > > I don't see the value in removing that code and documentation and losing
> > > functionality in order to simplify something that's already not complex.
> > 
> > Pub+barback is *incredibly* complex, and this is part of that system. Most
of
> > that complexity is necessary to support all the use cases we need to, but
it's
> > our job to eliminate any unnecessary complexity, wherever it exists. This is
> > unnecessary complexity.
> 
> Yes, and are you trying to argue that removing a flag (including removing its
> documentation) is the best use of my time for eliminating that complexity?

It would take about five minutes to remove the flag. It's right on the surface
of the user's interaction with the system, and its existence muddies the notion
of modes, so I think it's pretty low-hanging fruit for keeping complexity in
check.

> I don't know if users will use "pub serve --mode=release" much at all. I only
> added it because I was adding support for --mode to pub build so that users
> could do a build that included Dart files. Adding it to pub serve just seemed
> like a symmetric change.

I think most users will want to test their app in release mode before they ship
it. "pub serve" is a much easier way to do so than "pub build"ing it and putting
it on a server, especially if they may make some changes.

> > That's a use case where splitting "--minify" and "--mode" will cause actual
> > users actual pain. Is there a use-case where splitting them benefits users
> that
> > won't be handled by letting users refer to the dart2js transformer in the
> > pubspec?
> 
> I don't really know what the use cases are for --minify. We had a concrete use
> case where users wanted pub serve to not minify, and I added the flag at that
> time because it seemed useful to be able to configure it. We don't have the
> feedback to tell how useful it is.
> 
> I don't get why you're digging your heels in here. --minify is *already* in
the
> codebase. Why is this patch obligated to remove it?

There are two questions here. I think it's important to remove "--minify"
because the distinction between it and "--mode" is confusing and makes the
concept of modes harder to understand by tying some configuration options to
modes and others to pub commands. This is the most user-facing part of pub, and
the concepts behind transformers and modes are new to all our users; if this
isn't the time to maximize clarity of concepts, when is?

I don't think it's particularly important that *this patch* make the change. I
suggested it here because this patch introduces the confusion I described above
and it seemed like it would be an easy change to make. But as with any
(non-bug/style) code review comment, you're welcome to add a TODO instead of
doing it in this patch. I'll even write the patch for it if you'd like.

[Bob Nystrom, 2013-11-13 01:36:19]

https://codereview.chromium.org/69043003/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/command/build.dart (right):

https://codereview.chromium.org/69043003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/build.dart:45: help: 'Minify generated
JavaScript.');
On 2013/11/12 22:16:21, nweiz wrote:
> On 2013/11/12 21:28:06, Bob Nystrom wrote:
> > Right now, it's based on the command, which is a rough approximation of the
> > mode.
> 
> Why have a rough approximation when you could have the actual thing?
> 
> > Right, but we don't have a bunch of flags. We have two, one of which is
> already
> > submitted, tested, and documented.
> 
> I don't understand your point here. Are you saying that because one flag was
> already created, keeping them split provides better UI? That seems ridiculous
to
> me. The fact that code exists isn't in and of itself justification for keeping
> it around.
> 
> "--minify" was created before "--mode" existed. If we had written "--mode"
> first, there's no way we would have made "--minify" separate.
> 
> > I don't see the value in removing that code and documentation and losing
> > functionality in order to simplify something that's already not complex.
> 
> Pub+barback is *incredibly* complex, and this is part of that system. Most of
> that complexity is necessary to support all the use cases we need to, but it's
> our job to eliminate any unnecessary complexity, wherever it exists. This is
> unnecessary complexity.
> 
> As for the value, I've already explained it, but here it is again: users won't
> understand the distinction between "pub serve mode vs pub build mode" and
"debug
> mode vs release mode". They'll try to run "pub serve --mode=release" and
expect
> it to minify their code. It won't, and they'll be frustrated and unhappy.
> 
> That's a use case where splitting "--minify" and "--mode" will cause actual
> users actual pain. Is there a use-case where splitting them benefits users
that
> won't be handled by letting users refer to the dart2js transformer in the
> pubspec?

OK, I'm convinced. Done.

[nweiz, 2013-11-13 21:52:47]

lgtm

[Bob Nystrom, 2013-11-14 01:10:52]

Committed patchset #10 manually as r30262 (presubmit successful).