fix #626, add AMD module format and make it default

fix #626, add AMD module format and make it default

R=nweiz@google.com, vsm@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/f39c13e0837e3a884c4309ec18cf36a9d7620dd6

[Jennifer Messerly, 2016-08-16 20:19:39]

Description was changed from

==========
switch default module format to requirejs
==========

to

==========
fix #626, add requirejs module support and make it default
==========

[Jennifer Messerly, 2016-08-16 20:49:54]

jmesserly@google.com changed reviewers:
+ nweiz@google.com, vsm@google.com

[Jennifer Messerly, 2016-08-16 20:50:47]

BTW, I didn't remove module name yet, but we can totally do that once "legacy"
format is gone. Maybe should just get rid of it?

[vsm, 2016-08-16 21:23:23]

nice!

Will we be able to just concatenate js files together?  If not, what should be
our link step?  (Chrome is not very good at dealing with 1000s of files).

https://codereview.chromium.org/2249233002/diff/20001/karma.conf.js
File karma.conf.js (right):

https://codereview.chromium.org/2249233002/diff/20001/karma.conf.js#newcode16
karma.conf.js:16: {pattern: 'lib/runtime/dart_sdk.js', included: false},
What does "included" mean here?

https://codereview.chromium.org/2249233002/diff/20001/lib/runtime/dart_sdk.js
File lib/runtime/dart_sdk.js (right):

https://codereview.chromium.org/2249233002/diff/20001/lib/runtime/dart_sdk.js...
lib/runtime/dart_sdk.js:1: +define([], function() {
We should keep the legacy file in place to simplify the next roll.
Perhaps we should move to:

runtime/commonjs/dart_sdk.js
runtime/requirejs/dart_sdk.js

etc?

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/compil...
File lib/src/compiler/compiler.dart (right):

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/compil...
lib/src/compiler/compiler.dart:278: defaultsTo: 'requirejs')
We should keep the default until we fix downstream uses (which probably don't
set this explicitly right now).

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/module...
File lib/src/compiler/module_builder.dart (right):

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/module...
lib/src/compiler/module_builder.dart:166: // TODO(jmesserly): we could use
destructuring once Atom supports it.
nit: atom / electron would use common, not requirejs.  :-)

[Jennifer Messerly, 2016-08-16 21:29:28]

Link step --> whatever you'd use for JS code. There are several options. We
aren't treading new ground here.

https://codereview.chromium.org/2249233002/diff/20001/karma.conf.js
File karma.conf.js (right):

https://codereview.chromium.org/2249233002/diff/20001/karma.conf.js#newcode16
karma.conf.js:16: {pattern: 'lib/runtime/dart_sdk.js', included: false},
On 2016/08/16 21:23:23, vsm wrote:
> What does "included" mean here?

it's a karma thing:
http://karma-runner.github.io/1.0/config/files.html

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/compil...
File lib/src/compiler/compiler.dart (right):

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/compil...
lib/src/compiler/compiler.dart:278: defaultsTo: 'requirejs')
On 2016/08/16 21:23:23, vsm wrote:
> We should keep the default until we fix downstream uses (which probably don't
> set this explicitly right now).

it's really easy to tweak our internal build rules, if that's what you're
worried about. We can set the formal explicitly long before this flows in.

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/module...
File lib/src/compiler/module_builder.dart (right):

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/module...
lib/src/compiler/module_builder.dart:166: // TODO(jmesserly): we could use
destructuring once Atom supports it.
On 2016/08/16 21:23:23, vsm wrote:
> nit: atom / electron would use common, not requirejs.  :-)

thanks, I will clarify that comment. It's more "TODO we haven't bothered to use
destructuring anywhere yet because some impls didn't support it"

[Jennifer Messerly, 2016-08-16 21:34:45]

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/compil...
File lib/src/compiler/compiler.dart (right):

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/compil...
lib/src/compiler/compiler.dart:278: defaultsTo: 'requirejs')
On 2016/08/16 21:29:27, John Messerly wrote:
> On 2016/08/16 21:23:23, vsm wrote:
> > We should keep the default until we fix downstream uses (which probably
don't
> > set this explicitly right now).
> 
> it's really easy to tweak our internal build rules, if that's what you're
> worried about. We can set the formal explicitly long before this flows in.

I just sent you a CL that pins internal build rules to --modules=legacy

[nweiz, 2016-08-16 22:14:54]

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/code_g...
File lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:46: RequireJSModuleBuilder,
For what it's worth, despite their names, CommonJS and RequireJS aren't actually
the same type of thing. CommonJS is a standard, whereas RequireJS is an
implementation of a standard (namely, Asynchronous Module Definition). So it
might be better to call this AmdModuleBuilder and so on.

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:5300: 'moveNodesAfterSibling',
Nittiest of nits: for consistency, either remove the trailing comma here or add
one to the other similar lists.

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/compil...
File lib/src/compiler/compiler.dart (right):

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/compil...
lib/src/compiler/compiler.dart:276: 'requirejs': 'requirejs/amd modules'
Nit: add capitalization to the help strings here.

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/module...
File lib/src/compiler/module_builder.dart (right):

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/module...
lib/src/compiler/module_builder.dart:108: /// Generates commonjs modules (used
by node.js).
Nit: "CommonJS" and "Node.js"

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/module...
lib/src/compiler/module_builder.dart:174: assert(!importName.isStar); // import
* not supported yet.
Not supported by what?

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/module...
lib/src/compiler/module_builder.dart:186: assert(names != null); // export * not
supported in legacy modules.
This isn't a legacy module, right?

[nweiz, 2016-08-16 22:14:54]

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/module...
File lib/src/compiler/module_builder.dart (right):

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/module...
lib/src/compiler/module_builder.dart:186: assert(names != null); // export * not
supported in legacy modules.
This isn't a legacy module, right?

[Jennifer Messerly, 2016-08-22 19:50:18]

Thanks for the comments, I'm going to go through them, will let y'all know.

Vijay pointed out that we should generate & package several flavors of the SDK.
I'm going to address that too before I send out with revisions.

cheers!

[Jennifer Messerly, 2016-08-23 16:00:11]

Description was changed from

==========
fix #626, add requirejs module support and make it default
==========

to

==========
fix #626, add AMD module format and make it default
==========

[Jennifer Messerly, 2016-08-24 22:39:51]

Please take another look. In addition to addressing the comments, the big
changes are:
* compile SDK into multiple module format flavors,
* fix how we detect "notyetstrong" tests

I found a lot of unrelated-to-this-CL issues in how we detected build failures,
they're fixed in the new way of detecting failures:
* any test marked "fail" or "skip_fail" could have failed to compile, we didn't
see this
* any test with "negative_test" could fail to compile
* any test marked unittest could fail to compile silently
* subdirs like "corelib/regexp/*" appears to have been ignored

https://codereview.chromium.org/2249233002/diff/20001/lib/runtime/dart_sdk.js
File lib/runtime/dart_sdk.js (right):

https://codereview.chromium.org/2249233002/diff/20001/lib/runtime/dart_sdk.js...
lib/runtime/dart_sdk.js:1: +define([], function() {
On 2016/08/16 21:23:23, vsm wrote:
> We should keep the legacy file in place to simplify the next roll.
> Perhaps we should move to:
> 
> runtime/commonjs/dart_sdk.js
> runtime/requirejs/dart_sdk.js
> 
> etc?

done

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/code_g...
File lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:46: RequireJSModuleBuilder,
On 2016/08/16 22:14:53, nweiz wrote:
> For what it's worth, despite their names, CommonJS and RequireJS aren't
actually
> the same type of thing. CommonJS is a standard, whereas RequireJS is an
> implementation of a standard (namely, Asynchronous Module Definition). So it
> might be better to call this AmdModuleBuilder and so on.

yeah, I was kind of going back and forth there, as various terminology seems to
be used out there and it's shifted a bit over time. I'm happy to go with AMD
though, that's the term I'm more familiar with :)

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:5300: 'moveNodesAfterSibling',
On 2016/08/16 22:14:53, nweiz wrote:
> Nittiest of nits: for consistency, either remove the trailing comma here or
add
> one to the other similar lists.

definitely. Yeah I honestly have no idea what this code is for, certainly isn't
mine as I wouldn't have used a trailing comma in a 1 element list, nor would I
have LGTM'd a hard coded list of methods =(

I'm going to add some TODO's because this thing is terrible.

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/compil...
File lib/src/compiler/compiler.dart (right):

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/compil...
lib/src/compiler/compiler.dart:276: 'requirejs': 'requirejs/amd modules'
On 2016/08/16 22:14:53, nweiz wrote:
> Nit: add capitalization to the help strings here.

Done.

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/compil...
lib/src/compiler/compiler.dart:278: defaultsTo: 'requirejs')
On 2016/08/16 21:34:45, John Messerly wrote:
> On 2016/08/16 21:29:27, John Messerly wrote:
> > On 2016/08/16 21:23:23, vsm wrote:
> > > We should keep the default until we fix downstream uses (which probably
> don't
> > > set this explicitly right now).
> > 
> > it's really easy to tweak our internal build rules, if that's what you're
> > worried about. We can set the formal explicitly long before this flows in.
> 
> I just sent you a CL that pins internal build rules to --modules=legacy

update: and this was landed.

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/module...
File lib/src/compiler/module_builder.dart (right):

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/module...
lib/src/compiler/module_builder.dart:108: /// Generates commonjs modules (used
by node.js).
On 2016/08/16 22:14:54, nweiz wrote:
> Nit: "CommonJS" and "Node.js"

Done.

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/module...
lib/src/compiler/module_builder.dart:174: assert(!importName.isStar); // import
* not supported yet.
On 2016/08/16 22:14:54, nweiz wrote:
> Not supported by what?

clarified comment. the compiler doesn't emit `import *`, so we don't need to
support it here

https://codereview.chromium.org/2249233002/diff/20001/lib/src/compiler/module...
lib/src/compiler/module_builder.dart:186: assert(names != null); // export * not
supported in legacy modules.
On 2016/08/16 22:14:54, nweiz wrote:
> This isn't a legacy module, right?

clarified comment. The compiler doesn't emit `export *` (because of legacy
modules), so we don't need to support it.

[nweiz, 2016-08-24 23:33:26]

A bunch of nits, but otherwise LGTM!

https://codereview.chromium.org/2249233002/diff/40001/lib/src/compiler/code_g...
File lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/2249233002/diff/40001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:5279: //
Nit: unnecessary newline.

https://codereview.chromium.org/2249233002/diff/40001/lib/src/compiler/comman...
File lib/src/compiler/command.dart (right):

https://codereview.chromium.org/2249233002/diff/40001/lib/src/compiler/comman...
lib/src/compiler/command.dart:139: for (int i = 0; i < outPaths.length; i++) {
Nit: "var i"

https://codereview.chromium.org/2249233002/diff/40001/lib/src/compiler/comman...
lib/src/compiler/command.dart:143: // TODO(jmesserly): should we write the
summary next to every output?
IMO, yes. The extra time is likely to be negligible, and the ecosystem value of
knowing that every JS file has a corresponding summary will probably be high.

https://codereview.chromium.org/2249233002/diff/40001/lib/src/compiler/compil...
File lib/src/compiler/compiler.dart (right):

https://codereview.chromium.org/2249233002/diff/40001/lib/src/compiler/compil...
lib/src/compiler/compiler.dart:415: List list = new List.from(map['sources']);
Nit: "var list"

https://codereview.chromium.org/2249233002/diff/40001/lib/src/compiler/compil...
lib/src/compiler/compiler.dart:417: for (int i = 0; i < list.length; i++) {
Nit: "var i"

https://codereview.chromium.org/2249233002/diff/40001/lib/src/compiler/compil...
lib/src/compiler/compiler.dart:418: list[i] = path.relative(list[i], from: dir);
Source map locations are URIs, not local filesystem paths. You probably want:

    p.toUri(p.relative(p.fromUri(list[i]), from: dir))

https://codereview.chromium.org/2249233002/diff/40001/lib/src/compiler/compil...
lib/src/compiler/compiler.dart:420: return sourceMap;
I really don't like returning *and* modifying a parameter. It's too easy to look
at the signature and assume that it's safe to pass something that shouldn't be
modified.

https://codereview.chromium.org/2249233002/diff/40001/lib/src/compiler/module...
File lib/src/compiler/module_builder.dart (right):

https://codereview.chromium.org/2249233002/diff/40001/lib/src/compiler/module...
lib/src/compiler/module_builder.dart:13: /// EcmaScript 6 module using import
and export.
Nit: "ECMAScript" (also below)

My editor blood runs thick.

https://codereview.chromium.org/2249233002/diff/40001/lib/src/compiler/module...
lib/src/compiler/module_builder.dart:68: /// Transform an ES6 [module] into a
given module [format].
Nit: "Transforms"

https://codereview.chromium.org/2249233002/diff/40001/lib/src/compiler/module...
lib/src/compiler/module_builder.dart:76: Program lowerModuleFormat(ModuleFormat
format, Program module) {
I think "lower" is confusing here. Maybe "change" would be clearer?

[vsm, 2016-08-25 12:19:24]

lgtm

https://codereview.chromium.org/2249233002/diff/40001/lib/runtime/dart_librar...
File lib/runtime/dart_library.js (left):

https://codereview.chromium.org/2249233002/diff/40001/lib/runtime/dart_librar...
lib/runtime/dart_library.js:1: // Copyright (c) 2015, the Dart project authors. 
Please see the AUTHORS file
Not showing in the change, but I assume dart_sdk.js is gone from this dir as
well?

https://codereview.chromium.org/2249233002/diff/40001/lib/src/compiler/compil...
File lib/src/compiler/compiler.dart (right):

https://codereview.chromium.org/2249233002/diff/40001/lib/src/compiler/compil...
lib/src/compiler/compiler.dart:420: return sourceMap;
On 2016/08/24 23:33:26, nweiz wrote:
> I really don't like returning *and* modifying a parameter. It's too easy to
look
> at the signature and assume that it's safe to pass something that shouldn't be
> modified.

John: did you mean to return the new map (as per your method comment)?

https://codereview.chromium.org/2249233002/diff/40001/test-main.js
File test-main.js (right):

https://codereview.chromium.org/2249233002/diff/40001/test-main.js#newcode9
test-main.js:9: // syntax error:
A syntax error in the output?  Is this a DDC bug?

https://codereview.chromium.org/2249233002/diff/40001/test-main.js#newcode17
test-main.js:17: // module code execution error:
ditto

https://codereview.chromium.org/2249233002/diff/40001/test-main.js#newcode39
test-main.js:39: paths: {
slick!

https://codereview.chromium.org/2249233002/diff/40001/test/codegen_expected/j...
File test/codegen_expected/js_test.js (right):

https://codereview.chromium.org/2249233002/diff/40001/test/codegen_expected/j...
test/codegen_expected/js_test.js:5: throw Error("DDC Compilation Error: js_test
has 328 errors");
Should this have been generated?  I think this code is gone from
codegen_test.dart now.

https://codereview.chromium.org/2249233002/diff/40001/test/codegen_test.dart
File test/codegen_test.dart (right):

https://codereview.chromium.org/2249233002/diff/40001/test/codegen_test.dart#...
test/codegen_test.dart:149: expect(notStrong, isTrue,
Perhaps we should clear any file at the output path to remove stale output
(e.g., if a test becomes not-strong).

[Jennifer Messerly, 2016-08-25 16:21:40]

Thanks for the *excellent* comments Natalie & Vijay!

I think I got everything, so going to land, let me know if I missed anything &
can follow up. Cheers!

https://codereview.chromium.org/2249233002/diff/40001/lib/runtime/dart_librar...
File lib/runtime/dart_library.js (left):

https://codereview.chromium.org/2249233002/diff/40001/lib/runtime/dart_librar...
lib/runtime/dart_library.js:1: // Copyright (c) 2015, the Dart project authors. 
Please see the AUTHORS file
On 2016/08/25 12:19:24, vsm wrote:
> Not showing in the change, but I assume dart_sdk.js is gone from this dir as
> well?

weird. yes lib/runtime is totally gone, I did a `git mv` into the lib/js/amd
directory for history

https://codereview.chromium.org/2249233002/diff/40001/lib/src/compiler/code_g...
File lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/2249233002/diff/40001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:5279: //
On 2016/08/24 23:33:26, nweiz wrote:
> Nit: unnecessary newline.

there are not enough newlines to contain my rage at seeing this horrible code ;)
 Kidding :). Fixed

https://codereview.chromium.org/2249233002/diff/40001/lib/src/compiler/comman...
File lib/src/compiler/command.dart (right):

https://codereview.chromium.org/2249233002/diff/40001/lib/src/compiler/comman...
lib/src/compiler/command.dart:143: // TODO(jmesserly): should we write the
summary next to every output?
On 2016/08/24 23:33:26, nweiz wrote:
> IMO, yes. The extra time is likely to be negligible, and the ecosystem value
of
> knowing that every JS file has a corresponding summary will probably be high.

good idea. done!

https://codereview.chromium.org/2249233002/diff/40001/lib/src/compiler/compil...
File lib/src/compiler/compiler.dart (right):

https://codereview.chromium.org/2249233002/diff/40001/lib/src/compiler/compil...
lib/src/compiler/compiler.dart:415: List list = new List.from(map['sources']);
On 2016/08/24 23:33:26, nweiz wrote:
> Nit: "var list"

yeah this method just moved from somewhere. Never would've written it this way
in new code :)

but fixed.

https://codereview.chromium.org/2249233002/diff/40001/lib/src/compiler/compil...
lib/src/compiler/compiler.dart:417: for (int i = 0; i < list.length; i++) {
On 2016/08/24 23:33:26, nweiz wrote:
> Nit: "var i"

Done.

https://codereview.chromium.org/2249233002/diff/40001/lib/src/compiler/compil...
lib/src/compiler/compiler.dart:418: list[i] = path.relative(list[i], from: dir);
On 2016/08/24 23:33:26, nweiz wrote:
> Source map locations are URIs, not local filesystem paths. You probably want:
> 
>     p.toUri(p.relative(p.fromUri(list[i]), from: dir))

Done. The reason it was working: it only needs to handle source maps emitted by
our compiler. But that said I'm fine making it more correct in the general case.

https://codereview.chromium.org/2249233002/diff/40001/lib/src/compiler/compil...
lib/src/compiler/compiler.dart:420: return sourceMap;
On 2016/08/25 12:19:24, vsm wrote:
> On 2016/08/24 23:33:26, nweiz wrote:
> > I really don't like returning *and* modifying a parameter. It's too easy to
> look
> > at the signature and assume that it's safe to pass something that shouldn't
be
> > modified.
> 
> John: did you mean to return the new map (as per your method comment)?

good catch. yes it should be returning the copy. And I don't think we modify the
original map, but it does share structure for anything not mutated.

https://codereview.chromium.org/2249233002/diff/40001/lib/src/compiler/module...
File lib/src/compiler/module_builder.dart (right):

https://codereview.chromium.org/2249233002/diff/40001/lib/src/compiler/module...
lib/src/compiler/module_builder.dart:13: /// EcmaScript 6 module using import
and export.
On 2016/08/24 23:33:26, nweiz wrote:
> Nit: "ECMAScript" (also below)
> 
> My editor blood runs thick.

done :)  fixed all occurrences under dev_compiler/lib/

(there are some we pulled in that other people wrote in the Dart language tests,
I didn't fix those)

https://codereview.chromium.org/2249233002/diff/40001/lib/src/compiler/module...
lib/src/compiler/module_builder.dart:68: /// Transform an ES6 [module] into a
given module [format].
On 2016/08/24 23:33:26, nweiz wrote:
> Nit: "Transforms"

Done.

https://codereview.chromium.org/2249233002/diff/40001/lib/src/compiler/module...
lib/src/compiler/module_builder.dart:76: Program lowerModuleFormat(ModuleFormat
format, Program module) {
On 2016/08/24 23:33:26, nweiz wrote:
> I think "lower" is confusing here. Maybe "change" would be clearer?

hmmm, yeah that's tricky. "lower" is a bit of compiler jargon[1][2][3]. On the
one hand, I think that sort of thing is appropriate in a compiler for internal
APIs. But on the other, I'm definitely sympathetic to choosing plain language
wherever possible.

My problem with "change" is we can't change in all directions, if you lower to
AMD you can't go back to ES6. That's why "lower" suggests a direction: that
we're transforming from a "higher level" form to a "lower level" form, and
intuitively it's easy to go from "high level" to "low level" but not the
reverse.

Perhaps "transform" as the documentation comment uses ... while it's still not
suggesting a direction, it suggests we made a "dramatic change in the form"
which may remove the intuition that one can easily change back :)

[1]: https://ericlippert.com/2014/04/28/lowering-in-language-design-part-one/
[2]: http://www.cs.cornell.edu/courses/cs412/2003sp/lectures/lec18.pdf
[3]: https://gcc.gnu.org/onlinedocs/gccint/GENERIC.html

https://codereview.chromium.org/2249233002/diff/40001/test-main.js
File test-main.js (right):

https://codereview.chromium.org/2249233002/diff/40001/test-main.js#newcode9
test-main.js:9: // syntax error:
On 2016/08/25 12:19:24, vsm wrote:
> A syntax error in the output?  Is this a DDC bug?

yes, it's a bug. I happen to remember we already have one filed for incorrect
handling of labels.

https://codereview.chromium.org/2249233002/diff/40001/test-main.js#newcode17
test-main.js:17: // module code execution error:
On 2016/08/25 12:19:24, vsm wrote:
> ditto

yes, these are bugs, either in DDC or something Analyzer should reject (not
sure).

https://codereview.chromium.org/2249233002/diff/40001/test-main.js#newcode39
test-main.js:39: paths: {
On 2016/08/25 12:19:24, vsm wrote:
> slick!

thanks :). yeah requirejs and commonjs are nice. unfortunately it sounds like we
might have to change things to a relative paths as ES6 module loader is pretty
hobbled =(. But that's to address in a follow up, it's related to module/library
roots and that is unblocked by this change.

https://codereview.chromium.org/2249233002/diff/40001/test/codegen_expected/j...
File test/codegen_expected/js_test.js (right):

https://codereview.chromium.org/2249233002/diff/40001/test/codegen_expected/j...
test/codegen_expected/js_test.js:5: throw Error("DDC Compilation Error: js_test
has 328 errors");
On 2016/08/25 12:19:24, vsm wrote:
> Should this have been generated?  I think this code is gone from
> codegen_test.dart now.

good catch. we don't delete codegen_expect when something fails, unlike
gen/codegen_output. I'll revert.

https://codereview.chromium.org/2249233002/diff/40001/test/codegen_test.dart
File test/codegen_test.dart (right):

https://codereview.chromium.org/2249233002/diff/40001/test/codegen_test.dart#...
test/codegen_test.dart:149: expect(notStrong, isTrue,
On 2016/08/25 12:19:24, vsm wrote:
> Perhaps we should clear any file at the output path to remove stale output
> (e.g., if a test becomes not-strong).

It's not needed. test.sh does this at the start.

The reason you saw js_test is an artifact of my local iteration, and previous
states of this file. Now, if a codegen_expect thing stopped compiling, you would
see that as a test failure. Currently js_test is the only one that we can't
compile. (a very limited number of things are in codegen_expect)

[Jennifer Messerly, 2016-08-25 16:39:41]

Description was changed from

==========
fix #626, add AMD module format and make it default
==========

to

==========
fix #626, add AMD module format and make it default

R=nweiz@google.com, vsm@google.com

Committed:
https://github.com/dart-lang/dev_compiler/commit/f39c13e0837e3a884c4309ec18cf...
==========

[Jennifer Messerly, 2016-08-25 16:39:42]

Committed patchset #5 (id:60001) manually as
f39c13e0837e3a884c4309ec18cf36a9d7620dd6 (presubmit successful).