Add --modules=node support

Add --modules=node support
- Force import order in all sdk files + simplify module builders
- Stub a node_test.sh with hello world + DeltaBlue (to be expanded to language tests in a followup change)
- Use global_ from dart:_runtime in html lib
- Better export for symbols that node chokes upon: throw, const, void, implements, export... (define as throw_ locally, with proper local resolution, then export as throw).
- Cleanup node module builder

BUG=
R=jmesserly@google.com

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

[ochafik, 2016-01-26 08:56:53]

Patchset #5 (id:60001) has been deleted

[ochafik, 2016-01-26 08:56:58]

Patchset #4 (id:50001) has been deleted

[ochafik, 2016-01-26 08:57:03]

Patchset #3 (id:40001) has been deleted

[ochafik, 2016-01-26 09:02:16]

Patchset #3 (id:70001) has been deleted

[ochafik, 2016-01-26 09:14:29]

Description was changed from

==========
Add --modules=node support
- Use global_ from dart:_runtime in html lib
- Force import order in all sdk files + simplify module builders
- Stub a node_test.sh
- Cleanup node module builder
- Avoid symbols that node chokes upon: throw, const, void, implements, export...

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

to

==========
Add --modules=node support
- Force import order in all sdk files + simplify module builders
- Stub a node_test.sh with hello world + DeltaBlue (to be expanded to language
tests in a followup change)
- Use global_ from dart:_runtime in html lib
- Better export for symbols that node chokes upon: throw, const, void,
implements, export... (define as throw_ locally, with proper local resolution,
then export as throw).
- Cleanup node module builder

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

[ochafik, 2016-01-26 09:14:29]

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

[ochafik, 2016-01-26 09:16:28]

Hey guys, --module=node is alive!
Next task will be to checkin multiple runtimes, add language tests, and to
update devrun (right now the way one starts a main entry point is a bit weird,
see node_test.sh)

[ochafik, 2016-01-26 09:36:30]

Patchset #4 (id:90001) has been deleted

[ochafik, 2016-01-26 09:36:35]

Patchset #3 (id:80001) has been deleted

[vsm, 2016-01-26 14:09:41]

nice!

https://codereview.chromium.org/1633003002/diff/100001/lib/src/codegen/module...
File lib/src/codegen/module_builder.dart (right):

https://codereview.chromium.org/1633003002/diff/100001/lib/src/codegen/module...
lib/src/codegen/module_builder.dart:166: // js.statement("console.log('Loading '
+ #);", [js.string(jsPath)])
Remove?

https://codereview.chromium.org/1633003002/diff/100001/lib/src/codegen/module...
lib/src/codegen/module_builder.dart:171: var prioritaryExportItems =
<JS.ModuleItem>[];
s/prioritary/priority/ here and below?

https://codereview.chromium.org/1633003002/diff/100001/lib/src/codegen/module...
lib/src/codegen/module_builder.dart:175: emitExport(String name,
List<JS.ModuleItem> items) {
return type void?

https://codereview.chromium.org/1633003002/diff/100001/lib/src/codegen/module...
lib/src/codegen/module_builder.dart:181: getPrioritaryExportName(JS.ModuleItem
item) {
return type String?

https://codereview.chromium.org/1633003002/diff/100001/lib/src/codegen/module...
lib/src/codegen/module_builder.dart:188: return name;
Can you add a comment on why these need special casing?

https://codereview.chromium.org/1633003002/diff/100001/lib/src/compiler.dart
File lib/src/compiler.dart (right):

https://codereview.chromium.org/1633003002/diff/100001/lib/src/compiler.dart#...
lib/src/compiler.dart:517: 'dart.html',
These need to match the library names in html_ddc.dart and html_common.dart. 
Note how they turned into lazy imports in the generated code - it's failing to
match them with this list.

https://codereview.chromium.org/1633003002/diff/100001/tool/input_sdk/lib/htm...
File tool/input_sdk/lib/html/ddc/html_ddc.dart (right):

https://codereview.chromium.org/1633003002/diff/100001/tool/input_sdk/lib/htm...
tool/input_sdk/lib/html/ddc/html_ddc.dart:26: 
Note, I'm auto-generating this file right now.  Should be able to mimic all
these changes.

Is there a reason we need to use $global instead of window?  This library won't
work without the dom.

https://codereview.chromium.org/1633003002/diff/100001/tool/input_sdk/private...
File tool/input_sdk/private/classes.dart (right):

https://codereview.chromium.org/1633003002/diff/100001/tool/input_sdk/private...
tool/input_sdk/private/classes.dart:286: //let dartObjProto =
require('dart/core').Object.prototype; // \$Object.prototype;
Remove?

https://codereview.chromium.org/1633003002/diff/100001/tool/node_test.sh
File tool/node_test.sh (right):

https://codereview.chromium.org/1633003002/diff/100001/tool/node_test.sh#newc...
tool/node_test.sh:19: NODE_PATH=$output_dir node --harmony \
Do we need "--harmony"?  Looks like most features are just on by default:

https://nodejs.org/en/docs/es6/

https://codereview.chromium.org/1633003002/diff/100001/tool/node_test.sh#newc...
tool/node_test.sh:20: -e
"require('dart/_isolate_helper').startRootIsolate(require('$1').main, []);"
nit: line len

[ochafik, 2016-01-26 16:27:37]

Thanks Vijay!

https://codereview.chromium.org/1633003002/diff/100001/lib/src/codegen/module...
File lib/src/codegen/module_builder.dart (right):

https://codereview.chromium.org/1633003002/diff/100001/lib/src/codegen/module...
lib/src/codegen/module_builder.dart:166: // js.statement("console.log('Loading '
+ #);", [js.string(jsPath)])
On 2016/01/26 14:09:40, vsm wrote:
> Remove?

Done.

https://codereview.chromium.org/1633003002/diff/100001/lib/src/codegen/module...
lib/src/codegen/module_builder.dart:171: var prioritaryExportItems =
<JS.ModuleItem>[];
On 2016/01/26 14:09:41, vsm wrote:
> s/prioritary/priority/ here and below?

Acknowledged.

https://codereview.chromium.org/1633003002/diff/100001/lib/src/codegen/module...
lib/src/codegen/module_builder.dart:175: emitExport(String name,
List<JS.ModuleItem> items) {
On 2016/01/26 14:09:41, vsm wrote:
> return type void?

Acknowledged.

https://codereview.chromium.org/1633003002/diff/100001/lib/src/codegen/module...
lib/src/codegen/module_builder.dart:181: getPrioritaryExportName(JS.ModuleItem
item) {
On 2016/01/26 14:09:41, vsm wrote:
> return type String?

Acknowledged.

https://codereview.chromium.org/1633003002/diff/100001/lib/src/codegen/module...
lib/src/codegen/module_builder.dart:188: return name;
On 2016/01/26 14:09:40, vsm wrote:
> Can you add a comment on why these need special casing?

Actually, no longer needed with the forced order, simplified this method a lot.

https://codereview.chromium.org/1633003002/diff/100001/lib/src/compiler.dart
File lib/src/compiler.dart (right):

https://codereview.chromium.org/1633003002/diff/100001/lib/src/compiler.dart#...
lib/src/compiler.dart:517: 'dart.html',
On 2016/01/26 14:09:40, vsm wrote:
> These need to match the library names in html_ddc.dart and html_common.dart. 
> Note how they turned into lazy imports in the generated code - it's failing to
> match them with this list.

Done: added special casing for their module name (see getCorelibModuleName), as
the file names don't match the library.

https://codereview.chromium.org/1633003002/diff/100001/tool/input_sdk/lib/htm...
File tool/input_sdk/lib/html/ddc/html_ddc.dart (right):

https://codereview.chromium.org/1633003002/diff/100001/tool/input_sdk/lib/htm...
tool/input_sdk/lib/html/ddc/html_ddc.dart:26: 
On 2016/01/26 14:09:40, vsm wrote:
> Note, I'm auto-generating this file right now.  Should be able to mimic all
> these changes.
> 
> Is there a reason we need to use $global instead of window?  This library
won't
> work without the dom.

Platform.supportsTypedData will return true in node (where window is undefined)

Also, there are a couple of pure-JS DOM implementations for node.js (packages
jsdom, node-jsdom, node-dom...)

https://codereview.chromium.org/1633003002/diff/100001/tool/input_sdk/private...
File tool/input_sdk/private/classes.dart (right):

https://codereview.chromium.org/1633003002/diff/100001/tool/input_sdk/private...
tool/input_sdk/private/classes.dart:286: //let dartObjProto =
require('dart/core').Object.prototype; // \$Object.prototype;
On 2016/01/26 14:09:41, vsm wrote:
> Remove?

Done.

https://codereview.chromium.org/1633003002/diff/100001/tool/node_test.sh
File tool/node_test.sh (right):

https://codereview.chromium.org/1633003002/diff/100001/tool/node_test.sh#newc...
tool/node_test.sh:19: NODE_PATH=$output_dir node --harmony \
On 2016/01/26 14:09:40, vsm wrote:
> Do we need "--harmony"?  Looks like most features are just on by default:
> 
> https://nodejs.org/en/docs/es6/

Rest params (used in mixins) still need --harmony_rest_parameters (but I'm using
--harmony for simplicity :-))

https://codereview.chromium.org/1633003002/diff/100001/tool/node_test.sh#newc...
tool/node_test.sh:20: -e
"require('dart/_isolate_helper').startRootIsolate(require('$1').main, []);"
On 2016/01/26 14:09:40, vsm wrote:
> nit: line len

Done.

[ochafik, 2016-01-28 22:50:35]

Thanks Vijay!

[Jennifer Messerly, 2016-01-29 00:32:32]

LGTM. Only slight concern is js_names. Otherwise is great. Does the import order
thing work? If so, awesome!

https://codereview.chromium.org/1633003002/diff/170001/lib/src/codegen/js_cod...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1633003002/diff/170001/lib/src/codegen/js_cod...
lib/src/codegen/js_codegen.dart:192:
moduleBuilder.addImport(getCorelibModuleName(lib), null);
it would be better to use the URI here... rather than library name.

that said, we'd need to fix corelibOrder too.

https://codereview.chromium.org/1633003002/diff/170001/lib/src/codegen/js_nam...
File lib/src/codegen/js_names.dart (right):

https://codereview.chromium.org/1633003002/diff/170001/lib/src/codegen/js_nam...
lib/src/codegen/js_names.dart:285: // Node refuses these:
Any more info on these? What's the error message say, and under what conditions
do they not work?

it's a little scary, usually JS is careful to not take away names that worked,
it breaks backwards compat. Hence a lot of names are only blacklisted in "strict
mode"

(we try and be careful to only blacklist names when we absolutely need to.)

https://codereview.chromium.org/1633003002/diff/170001/lib/src/codegen/js_nam...
lib/src/codegen/js_names.dart:286: case "async":
this could be related to https://tc39.github.io/ecmascript-asyncawait/, but I'd
be surprised if it isn't backwards compatible.

https://codereview.chromium.org/1633003002/diff/170001/lib/src/codegen/js_nam...
lib/src/codegen/js_names.dart:288: case "void":
it looks like "void" is already covered, above? So this is a duplicate case

https://codereview.chromium.org/1633003002/diff/170001/lib/src/codegen/module...
File lib/src/codegen/module_builder.dart (right):

https://codereview.chromium.org/1633003002/diff/170001/lib/src/codegen/module...
lib/src/codegen/module_builder.dart:193: // TODO(ochafik): What to do of
jsModuleValue?
Wording suggestion: "what to do with jsModuleValue"


I believe the semantics of it are "attach APIs to this existing module" ... so
we'd generate something like:

let exports = require('module_we_want_to_extend');
exports.SomeNewAPI = ...

In practice it's used for "window" so doesn't make sense on Node right now.

I'm not super comfortable with monkey-patching another module, anyway. I think
we should revisit jsModuleValue mechanism.

https://codereview.chromium.org/1633003002/diff/170001/lib/src/compiler.dart
File lib/src/compiler.dart (right):

https://codereview.chromium.org/1633003002/diff/170001/lib/src/compiler.dart#...
lib/src/compiler.dart:523: String getCorelibModuleName(String lib) {
doc comment? :)
https://www.dartlang.org/effective-dart/documentation/#prefer-writing-doc-com...

https://codereview.chromium.org/1633003002/diff/170001/lib/src/compiler.dart#...
lib/src/compiler.dart:530: assert(lib.startsWith('dart.'));
this assert could be moved up top?

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

https://codereview.chromium.org/1633003002/diff/170001/lib/src/options.dart#n...
lib/src/options.dart:319: getEnumName(ModuleFormat.node): 'node.js modules'
https://nodejs.org/api/modules.html

is a good link if you want to add it

[ochafik, 2016-01-29 09:38:17]

Thanks John!

https://codereview.chromium.org/1633003002/diff/170001/lib/src/codegen/js_cod...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1633003002/diff/170001/lib/src/codegen/js_cod...
lib/src/codegen/js_codegen.dart:192:
moduleBuilder.addImport(getCorelibModuleName(lib), null);
On 2016/01/29 00:32:31, John Messerly wrote:
> it would be better to use the URI here... rather than library name.
> 
> that said, we'd need to fix corelibOrder too.

Good idea, left myself a todo :-)

https://codereview.chromium.org/1633003002/diff/170001/lib/src/codegen/js_nam...
File lib/src/codegen/js_names.dart (right):

https://codereview.chromium.org/1633003002/diff/170001/lib/src/codegen/js_nam...
lib/src/codegen/js_names.dart:285: // Node refuses these:
On 2016/01/29 00:32:31, John Messerly wrote:
> Any more info on these? What's the error message say, and under what
conditions
> do they not work?
> 
> it's a little scary, usually JS is careful to not take away names that worked,
> it breaks backwards compat. Hence a lot of names are only blacklisted in
"strict
> mode"
> 
> (we try and be careful to only blacklist names when we absolutely need to.)

Dropped these, not needed now...

https://codereview.chromium.org/1633003002/diff/170001/lib/src/codegen/js_nam...
lib/src/codegen/js_names.dart:286: case "async":
On 2016/01/29 00:32:31, John Messerly wrote:
> this could be related to https://tc39.github.io/ecmascript-asyncawait/, but
I'd
> be surprised if it isn't backwards compatible.

My bad, now fixed (I had a weird collision of async lib import alias and async
function declaration, sometime before the exportName -> name fix)

https://codereview.chromium.org/1633003002/diff/170001/lib/src/codegen/js_nam...
lib/src/codegen/js_names.dart:288: case "void":
On 2016/01/29 00:32:31, John Messerly wrote:
> it looks like "void" is already covered, above? So this is a duplicate case

Done.

https://codereview.chromium.org/1633003002/diff/170001/lib/src/codegen/module...
File lib/src/codegen/module_builder.dart (right):

https://codereview.chromium.org/1633003002/diff/170001/lib/src/codegen/module...
lib/src/codegen/module_builder.dart:193: // TODO(ochafik): What to do of
jsModuleValue?
On 2016/01/29 00:32:31, John Messerly wrote:
> Wording suggestion: "what to do with jsModuleValue"
> 
> 
> I believe the semantics of it are "attach APIs to this existing module" ... so
> we'd generate something like:
> 
> let exports = require('module_we_want_to_extend');
> exports.SomeNewAPI = ...
> 
> In practice it's used for "window" so doesn't make sense on Node right now.
> 
> I'm not super comfortable with monkey-patching another module, anyway. I think
> we should revisit jsModuleValue mechanism.
> 

Thanks for the explanation! Upgraded the comment

https://codereview.chromium.org/1633003002/diff/170001/lib/src/compiler.dart
File lib/src/compiler.dart (right):

https://codereview.chromium.org/1633003002/diff/170001/lib/src/compiler.dart#...
lib/src/compiler.dart:523: String getCorelibModuleName(String lib) {
On 2016/01/29 00:32:32, John Messerly wrote:
> doc comment? :)
>
https://www.dartlang.org/effective-dart/documentation/#prefer-writing-doc-com...

Done.

https://codereview.chromium.org/1633003002/diff/170001/lib/src/compiler.dart#...
lib/src/compiler.dart:530: assert(lib.startsWith('dart.'));
On 2016/01/29 00:32:32, John Messerly wrote:
> this assert could be moved up top?

Done (+ turned into `assert(corelibOrder.contains(lib));`)

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

https://codereview.chromium.org/1633003002/diff/170001/lib/src/options.dart#n...
lib/src/options.dart:319: getEnumName(ModuleFormat.node): 'node.js modules'
On 2016/01/29 00:32:32, John Messerly wrote:
> https://nodejs.org/api/modules.html
> 
> is a good link if you want to add it

Thanks!

[ochafik, 2016-01-29 09:41:25]

FYI: the import order trick worked like a charm, just had to insert those phony
requires in the right order!

It's not quite as easy for Closure, unfortunately, as it actually fights back
against cyclic deps... but when there's a will, there's a way ;-)

[ochafik, 2016-01-29 10:01:49]

Description was changed from

==========
Add --modules=node support
- Force import order in all sdk files + simplify module builders
- Stub a node_test.sh with hello world + DeltaBlue (to be expanded to language
tests in a followup change)
- Use global_ from dart:_runtime in html lib
- Better export for symbols that node chokes upon: throw, const, void,
implements, export... (define as throw_ locally, with proper local resolution,
then export as throw).
- Cleanup node module builder

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

to

==========
Add --modules=node support
- Force import order in all sdk files + simplify module builders
- Stub a node_test.sh with hello world + DeltaBlue (to be expanded to language
tests in a followup change)
- Use global_ from dart:_runtime in html lib
- Better export for symbols that node chokes upon: throw, const, void,
implements, export... (define as throw_ locally, with proper local resolution,
then export as throw).
- Cleanup node module builder

BUG=
R=jmesserly@google.com

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

[ochafik, 2016-01-29 10:01:51]

Committed patchset #11 (id:210001) manually as
393c2b3b7212f94e629ed47e0a64b4183ee5a79f (presubmit successful).