Initial --modules=es6 support

Initial --modules=es6 support

Known issues:
- Import syntax: using 'dart/core' right now, but should be
'./dart/core' or something else?
- _jsModuleValue not supported in es6 output yet (how is it meant to be
used?)

BUG=
R=jmesserly@google.com

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

[ochafik, 2016-01-21 16:46:11]

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

[ochafik, 2016-01-21 16:46:11]

Hey guys, I had a go at outputting es6 modules: there are some unresolved
questions (import path) and I still have to handle the case of top-level vars,
but I'd like to hear if you think this is going in the right direction :-)

Cheers

[ochafik, 2016-01-21 17:20:37]

Description was changed from

==========
Initial --modules=es6 support

Known issues:
- Mutable top-level vars are not supported yet: will refactor
_addExport to know when to create a getter+setter in exports
- Import syntax: using 'dart/core' right now, but should be
'./dart/core' or something else?
- _jsModuleValue not supported in es6 output yet (how is it meant to be
used?)

BUG=
==========

to

==========
Initial --modules=es6 support

Known issues:
- Import syntax: using 'dart/core' right now, but should be
'./dart/core' or something else?
- _jsModuleValue not supported in es6 output yet (how is it meant to be
used?)

BUG=
==========

[vsm, 2016-01-21 19:19:31]

On 2016/01/21 16:46:11, ochafik wrote:
> Hey guys, I had a go at outputting es6 modules: there are some unresolved
> questions (import path) and I still have to handle the case of top-level vars,
> but I'd like to hear if you think this is going in the right direction :-)
> 
> Cheers

QQ: are you running ./tools/presubmit.sh?  That should fix some of the
formatting issues here, I think.

[ochafik, 2016-01-22 09:14:39]

On 2016/01/21 19:19:31, vsm wrote:
> On 2016/01/21 16:46:11, ochafik wrote:
> > Hey guys, I had a go at outputting es6 modules: there are some unresolved
> > questions (import path) and I still have to handle the case of top-level
vars,
> > but I'd like to hear if you think this is going in the right direction :-)
> > 
> > Cheers
> 
> QQ: are you running ./tools/presubmit.sh?  That should fix some of the
> formatting issues here, I think.

Fixed, thanks!

[Jennifer Messerly, 2016-01-22 18:29:01]

LGTM!

https://codereview.chromium.org/1612083002/diff/60001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1612083002/diff/60001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:182: var moduleBuilder = new ModuleBuilder(
wow, this ModuleBuilder is a really clean pattern! Love how it makes the code
here so easy to reason about.

https://codereview.chromium.org/1612083002/diff/60001/lib/src/codegen/module_...
File lib/src/codegen/module_builder.dart (right):

https://codereview.chromium.org/1612083002/diff/60001/lib/src/codegen/module_...
lib/src/codegen/module_builder.dart:5: library
dev_compiler.src.codegen.js_codegen;
fix library name here?

BTW, I heard we can delete library names now, so I may go through en masse and
delete :D

https://codereview.chromium.org/1612083002/diff/60001/lib/src/codegen/module_...
lib/src/codegen/module_builder.dart:11: abstract class ModuleBuilder {
Perhaps add a doc comment to this?

https://codereview.chromium.org/1612083002/diff/60001/lib/src/codegen/module_...
lib/src/codegen/module_builder.dart:24: return new DartModuleBuilder(jsPath,
_jsModuleValue, _exportsVar);
I wonder if we could factor this so given an enum value, we can just look up a
method to construct it? I guess without constructor tear offs it's a bit
awkward.

https://codereview.chromium.org/1612083002/diff/60001/lib/src/codegen/module_...
lib/src/codegen/module_builder.dart:27: default:
I think if you leave off "default", we'll give a warning from Analyzer if we
forget a case.

(static failure being preferable to runtime failure, IMO)

https://codereview.chromium.org/1612083002/diff/60001/lib/src/codegen/module_...
lib/src/codegen/module_builder.dart:46: final bool isLazy;
Maybe we should document that this field is only for legacy Dart format.
Feel free to add something like

// TODO(jmesserly): remove this, it's historical and we shouldn't need it even
in our legacy module format.

https://codereview.chromium.org/1612083002/diff/60001/lib/src/codegen/module_...
lib/src/codegen/module_builder.dart:51: class DartModuleBuilder extends
ModuleBuilder {
Rename to LegacyModuleBuilder?

https://codereview.chromium.org/1612083002/diff/60001/lib/src/codegen/module_...
lib/src/codegen/module_builder.dart:101: /// TODO(ochafik): Break strong dep
cycles to accommodate the Closure Compiler.
trivial style nit, TODO should not be in a doc comment. So two-slashes.

Maybe it's not worth going into now, but at some point we should discuss the
Closure Compiler issue in more detail? We may need a separate module format for
it, depending on what compromises to readability (vs hand written ES6 code) are
necessary.

I definitely hope we can use the ES6 module format as our primary output.

https://codereview.chromium.org/1612083002/diff/60001/lib/src/codegen/module_...
lib/src/codegen/module_builder.dart:107: // TODO(jmesserly): it would be great
to run the renamer on the body,
This TODO should move down to the `for (var i in _imports) {` block?

https://codereview.chromium.org/1612083002/diff/60001/lib/src/codegen/module_...
lib/src/codegen/module_builder.dart:112: moduleStatements.add(js.statement("'use
strict';"));
"use strict" is not needed in ES6 modules:
http://www.ecma-international.org/ecma-262/6.0/#sec-strict-mode-code
> Module code is always strict mode code.

https://codereview.chromium.org/1612083002/diff/60001/lib/src/codegen/module_...
lib/src/codegen/module_builder.dart:117: // TODO(ochafik): laziness, late
binding, etc, to support Closure...
well I hope we don't need lazy imports, because no such concept exists in ES6 :)

To be honest, I don't think it really does anything functional in the current
module system. (It's just an assertion, basically, to catch if the compiler
messed up its dependency order.)

https://codereview.chromium.org/1612083002/diff/60001/lib/src/codegen/module_...
lib/src/codegen/module_builder.dart:139: Iterable<JS.ModuleItem>
_flattenBlocks(List<JS.ModuleItem> stats) =>
BTW, I wonder if we should add something like this to JS.Block, since it's come
up as useful in a few places?

https://codereview.chromium.org/1612083002/diff/60001/lib/src/options.dart
File lib/src/options.dart (right):

https://codereview.chromium.org/1612083002/diff/60001/lib/src/options.dart#ne...
lib/src/options.dart:59: enum ModuleFormat { es6, dart }
Heh, not sure the enum adds much value. More an issue with Dart enums, though.

e.g. with a custom enum class, we could add a method that knows how to
instantiate the right module builder, for example. And we wouldn't need to make
its internal name exactly match its option representation. And the parse method
wouldn't need to live as another top-level name.

But, we'd lose out on the static checking that all cases are covered in switch,
maybe.

So, not sure this is worth changing. Just throwing that out there for
discussion. :)

https://codereview.chromium.org/1612083002/diff/60001/lib/src/options.dart#ne...
lib/src/options.dart:62: ModuleFormat.values.firstWhere((v) => s ==
'$v'.split('.')[1]);
if parsing fails, are we okay because args option parser will have already
issued a nice error message before we get to this point?

https://codereview.chromium.org/1612083002/diff/60001/lib/src/options.dart#ne...
lib/src/options.dart:317: 'dart': 'a custom format used by dartdevc, similar to
AMD'
Hmmm, I don't like calling this format "dart" ... as it implies Dart needs its
own module format, or something.
Perhaps "legacy", to make it clear this is historical baggage?

https://codereview.chromium.org/1612083002/diff/60001/test/codegen_test.dart
File test/codegen_test.dart (right):

https://codereview.chromium.org/1612083002/diff/60001/test/codegen_test.dart#...
test/codegen_test.dart:155: filename == 'es6_modules' ? ModuleFormat.es6 :
ModuleFormat.dart;
nice! I'll see if I can help clean this up (my TODO above). Don't let it block
you. This was totally the right thing to do given the state of codegen_test.

[ochafik, 2016-01-23 12:24:18]

Thanks John!

https://codereview.chromium.org/1612083002/diff/60001/lib/src/codegen/module_...
File lib/src/codegen/module_builder.dart (right):

https://codereview.chromium.org/1612083002/diff/60001/lib/src/codegen/module_...
lib/src/codegen/module_builder.dart:5: library
dev_compiler.src.codegen.js_codegen;
On 2016/01/22 18:29:01, John Messerly wrote:
> fix library name here?
> 
> BTW, I heard we can delete library names now, so I may go through en masse and
> delete :D

Done.

https://codereview.chromium.org/1612083002/diff/60001/lib/src/codegen/module_...
lib/src/codegen/module_builder.dart:11: abstract class ModuleBuilder {
On 2016/01/22 18:29:01, John Messerly wrote:
> Perhaps add a doc comment to this?

Done.

https://codereview.chromium.org/1612083002/diff/60001/lib/src/codegen/module_...
lib/src/codegen/module_builder.dart:24: return new DartModuleBuilder(jsPath,
_jsModuleValue, _exportsVar);
On 2016/01/22 18:29:01, John Messerly wrote:
> I wonder if we could factor this so given an enum value, we can just look up a
> method to construct it? I guess without constructor tear offs it's a bit
> awkward.

Yes, I did that in the first version
(https://codereview.chromium.org/1612083002/patch/1/10002), but the tearoffs
were driving the formatter (and analyzer) crazy. Also, it would require some
cumbersome typedef to make this typesafe...

https://codereview.chromium.org/1612083002/diff/60001/lib/src/codegen/module_...
lib/src/codegen/module_builder.dart:27: default:
On 2016/01/22 18:29:01, John Messerly wrote:
> I think if you leave off "default", we'll give a warning from Analyzer if we
> forget a case.
> 
> (static failure being preferable to runtime failure, IMO)

Ah, sweet!

https://codereview.chromium.org/1612083002/diff/60001/lib/src/codegen/module_...
lib/src/codegen/module_builder.dart:46: final bool isLazy;
On 2016/01/22 18:29:01, John Messerly wrote:
> Maybe we should document that this field is only for legacy Dart format.
> Feel free to add something like
> 
> // TODO(jmesserly): remove this, it's historical and we shouldn't need it even
> in our legacy module format.

Not sure we can remove it, if only for Closure, added this todo:

  // TODO(jmesserly): Assess whether we can remove this (we shouldn't need it
  // even in our legacy module format, but it might still be useful for Closure
  // with ES6 modules).

https://codereview.chromium.org/1612083002/diff/60001/lib/src/codegen/module_...
lib/src/codegen/module_builder.dart:51: class DartModuleBuilder extends
ModuleBuilder {
On 2016/01/22 18:29:01, John Messerly wrote:
> Rename to LegacyModuleBuilder?

Done.

https://codereview.chromium.org/1612083002/diff/60001/lib/src/codegen/module_...
lib/src/codegen/module_builder.dart:101: /// TODO(ochafik): Break strong dep
cycles to accommodate the Closure Compiler.
On 2016/01/22 18:29:01, John Messerly wrote:
> trivial style nit, TODO should not be in a doc comment. So two-slashes.
> 
> Maybe it's not worth going into now, but at some point we should discuss the
> Closure Compiler issue in more detail? We may need a separate module format
for
> it, depending on what compromises to readability (vs hand written ES6 code)
are
> necessary.
> 
> I definitely hope we can use the ES6 module format as our primary output.

Absolutely! I think the only diff between ES6ModuleBuilder and
ClosureModuleBuilder will be special handling of lazy imports. I'll start
experimenting / will have more material to discuss as soon as this goes in :-)

https://codereview.chromium.org/1612083002/diff/60001/lib/src/codegen/module_...
lib/src/codegen/module_builder.dart:107: // TODO(jmesserly): it would be great
to run the renamer on the body,
On 2016/01/22 18:29:01, John Messerly wrote:
> This TODO should move down to the `for (var i in _imports) {` block?

Done.

https://codereview.chromium.org/1612083002/diff/60001/lib/src/codegen/module_...
lib/src/codegen/module_builder.dart:112: moduleStatements.add(js.statement("'use
strict';"));
On 2016/01/22 18:29:01, John Messerly wrote:
> "use strict" is not needed in ES6 modules:
> http://www.ecma-international.org/ecma-262/6.0/#sec-strict-mode-code
> > Module code is always strict mode code.

The power of specs! Thanks!

https://codereview.chromium.org/1612083002/diff/60001/lib/src/codegen/module_...
lib/src/codegen/module_builder.dart:117: // TODO(ochafik): laziness, late
binding, etc, to support Closure...
On 2016/01/22 18:29:01, John Messerly wrote:
> well I hope we don't need lazy imports, because no such concept exists in ES6
:)
> 
> To be honest, I don't think it really does anything functional in the current
> module system. (It's just an assertion, basically, to catch if the compiler
> messed up its dependency order.)

So, I think we'll need some combination of lazy classes, lazy imports, or
breaking each class out to its own module: non-lazy classes need their parent to
be defined when their declaration is evaluated, and references from their bodies
may create import cycles.

I think the runtime dependency order largely depends on which files are imported
first, which is partly out of the compiler's control.

I strongly hope I'm wrong here, can't wait to chat more about this :-)

https://codereview.chromium.org/1612083002/diff/60001/lib/src/codegen/module_...
lib/src/codegen/module_builder.dart:139: Iterable<JS.ModuleItem>
_flattenBlocks(List<JS.ModuleItem> stats) =>
On 2016/01/22 18:29:01, John Messerly wrote:
> BTW, I wonder if we should add something like this to JS.Block, since it's
come
> up as useful in a few places?

On second thought, I think this is wrong *in general* (breaks block scoping),
although desirable in the special case of module building (left a TODO, will
address in the backend CL).

https://codereview.chromium.org/1612083002/diff/60001/lib/src/options.dart
File lib/src/options.dart (right):

https://codereview.chromium.org/1612083002/diff/60001/lib/src/options.dart#ne...
lib/src/options.dart:59: enum ModuleFormat { es6, dart }
On 2016/01/22 18:29:01, John Messerly wrote:
> Heh, not sure the enum adds much value. More an issue with Dart enums, though.
> 
> e.g. with a custom enum class, we could add a method that knows how to
> instantiate the right module builder, for example. And we wouldn't need to
make
> its internal name exactly match its option representation. And the parse
method
> wouldn't need to live as another top-level name.
> 
> But, we'd lose out on the static checking that all cases are covered in
switch,
> maybe.
> 
> So, not sure this is worth changing. Just throwing that out there for
> discussion. :)

I agree, *if only* we had Java enums...

https://codereview.chromium.org/1612083002/diff/60001/lib/src/options.dart#ne...
lib/src/options.dart:62: ModuleFormat.values.firstWhere((v) => s ==
'$v'.split('.')[1]);
On 2016/01/22 18:29:01, John Messerly wrote:
> if parsing fails, are we okay because args option parser will have already
> issued a nice error message before we get to this point?

Yes we are, but cleaned up utils (moved to utils.dart) + added tests to stay on
the safe side.

https://codereview.chromium.org/1612083002/diff/60001/lib/src/options.dart#ne...
lib/src/options.dart:317: 'dart': 'a custom format used by dartdevc, similar to
AMD'
On 2016/01/22 18:29:01, John Messerly wrote:
> Hmmm, I don't like calling this format "dart" ... as it implies Dart needs its
> own module format, or something.
> Perhaps "legacy", to make it clear this is historical baggage?
> 
> 

Done (+ tied these values to the enum with a getEnumName)

[ochafik, 2016-01-24 18:10:00]

Description was changed from

==========
Initial --modules=es6 support

Known issues:
- Import syntax: using 'dart/core' right now, but should be
'./dart/core' or something else?
- _jsModuleValue not supported in es6 output yet (how is it meant to be
used?)

BUG=
==========

to

==========
Initial --modules=es6 support

Known issues:
- Import syntax: using 'dart/core' right now, but should be
'./dart/core' or something else?
- _jsModuleValue not supported in es6 output yet (how is it meant to be
used?)

BUG=
R=jmesserly@google.com

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

[ochafik, 2016-01-24 18:10:04]

Committed patchset #5 (id:80001) manually as
9e8845249466b3a27364c28a68e46a6999647559 (presubmit successful).