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

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

Review URL: https://codereview.chromium.org/2249233002 .

Author: John Messerly

pkg/dev_compiler/.gitignore

@@ -17,8 +17,8 @@ doc/api/
 # Generated files go here.
 gen/
 
-# Ignore generated summary.
-lib/runtime/dart_sdk.sum
+# Ignore generated SDK summary.
+lib/js/*/*.sum
 
 # Created by ./tool/dependency_overrides.sh.
 dependency_overrides/

pkg/dev_compiler/CHANGELOG.md

@@ -1,5 +1,10 @@
 # dev_compiler changelog
 
+## next release
+- add support for AMD modules and make it the default.
+- precompile the SDK in AMD, CommonJS, and ES6 flavors.
+- legacy module format is deprecated.
+
 ## 0.1.24
 - workaround breaking change on requestAnimationFrame
 

pkg/dev_compiler/karma.conf.js

@@ -13,26 +13,17 @@ module.exports = function(config) {
 
     // list of files / patterns to load in the browser
     files: [
-      'lib/runtime/dart_*.js',
-      // {pattern: 'test/browser/*.js', included: false}
-      'gen/codegen_output/pkg/*.js',
-      'gen/codegen_output/language/**.js',
-      'gen/codegen_output/language/**.err',
-      'gen/codegen_output/corelib/**.js',
-      'gen/codegen_output/corelib/**.err',
-      'gen/codegen_output/lib/convert/**.js',
-      'gen/codegen_output/lib/html/**.js',
-      'gen/codegen_output/lib/math/**.js',
-      'gen/codegen_output/lib/mirrors/**.js',
-      'gen/codegen_output/lib/typed_data/**.js',
-      'gen/codegen_output/lib/*/**.err',
-      'test/browser/*.js',
+      {pattern: 'lib/js/amd/dart_sdk.js', included: false},
+      {pattern: 'gen/codegen_output/pkg/*.js', included: false},
+      {pattern: 'gen/codegen_output/language/**/*.js', included: false},
+      {pattern: 'gen/codegen_output/corelib/**/*.js', included: false},
+      {pattern: 'gen/codegen_output/lib/**/*.js', included: false},
+      {pattern: 'test/browser/*.js', included: false},
       'test-main.js',
     ],
 
     // list of files to exclude
-    exclude: [
-    ],
+    exclude: [],
 
     // preprocess matching files before serving them to the browser
     // available preprocessors: https://npmjs.org/browse/keyword/karma-preprocessor

pkg/dev_compiler/lib/js/amd/dart_sdk.js

@@ -1466,7 +1466,7 @@ dart_library.library('dart_sdk', null, /* Imports */[
     let src = '';
     for (let i = 2; i < dart.notNull(stack[dartx.length]); ++i) {
       let frame = stack[dartx.get](i);
-      if (!dart.test(frame[dartx.contains]('dev_compiler/lib/runtime/dart_sdk.js'))) {
+      if (!dart.test(frame[dartx.contains]('dart_sdk.js'))) {
         src = frame;
         break;
       }

pkg/dev_compiler/lib/js/common/dart_sdk.js

@@ -23,15 +23,14 @@ import 'package:analyzer/src/summary/summarize_elements.dart'
     show PackageBundleAssembler;
 import 'package:analyzer/src/task/strong/ast_properties.dart'
     show isDynamicInvoke, setIsDynamicInvoke;
-import 'package:source_maps/source_maps.dart';
 import 'package:path/path.dart' show separator;
 
 import '../closure/closure_annotator.dart' show ClosureAnnotator;
 import '../js_ast/js_ast.dart' as JS;
 import '../js_ast/js_ast.dart' show js;
 import 'ast_builder.dart' show AstBuilder;
 import 'compiler.dart'
-    show BuildUnit, CompilerOptions, JSModuleFile, ModuleFormat;
+    show BuildUnit, CompilerOptions, JSModuleFile;
 import 'element_helpers.dart';
 import 'element_loader.dart' show ElementLoader;
 import 'extension_types.dart' show ExtensionTypeSet;
@@ -40,12 +39,10 @@ import 'js_interop.dart';
 import 'js_metalet.dart' as JS;
 import 'js_names.dart' as JS;
 import 'js_typeref_codegen.dart' show JsTypeRefCodegen;
-import 'module_builder.dart'
-    show LegacyModuleBuilder, NodeModuleBuilder, pathToJSIdentifier;
+import 'module_builder.dart' show pathToJSIdentifier;
 import 'nullable_type_inference.dart' show NullableTypeInference;
 import 'reify_coercions.dart' show CoercionReifier;
 import 'side_effect_analysis.dart' show ConstFieldVisitor, isStateless;
-import 'source_map_printer.dart' show SourceMapPrintingContext;
 import 'type_utilities.dart';
 
 class CodeGenerator extends GeneralizingAstVisitor
@@ -173,46 +170,21 @@ class CodeGenerator extends GeneralizingAstVisitor
       _libraryRoot = '$_libraryRoot${separator}';
     }
 
-    var jsTree = _emitModule(compilationUnits);
-    var codeAndSourceMap = _writeJSText(unit, jsTree);
+    var module = _emitModule(compilationUnits);
+    var dartApiSummary = _summarizeModule(compilationUnits);
 
-    List<int> summary;
-    if (options.summarizeApi) {
-      var assembler = new PackageBundleAssembler();
-      compilationUnits
-          .map((u) => u.element.library)
-          .toSet()
-          .forEach(assembler.serializeLibraryElement);
-      summary = assembler.assemble().toBuffer();
-    }
-
-    return new JSModuleFile(
-        unit.name, errors, codeAndSourceMap.e0, codeAndSourceMap.e1, summary);
+    return new JSModuleFile(unit.name, errors, options, module, dartApiSummary);
   }
 
-  Tuple2<String, Map> _writeJSText(BuildUnit unit, JS.Program jsTree) {
-    var opts = new JS.JavaScriptPrintingOptions(
-        emitTypes: options.closure,
-        allowKeywordsInProperties: true,
-        allowSingleLineIfStatements: true);
-    JS.SimpleJavaScriptPrintingContext printer;
-    SourceMapBuilder sourceMap;
-    if (options.sourceMap) {
-      var sourceMapContext = new SourceMapPrintingContext();
-      sourceMap = sourceMapContext.sourceMap;
-      printer = sourceMapContext;
-    } else {
-      printer = new JS.SimpleJavaScriptPrintingContext();
-    }
-
-    jsTree.accept(new JS.Printer(opts, printer,
-        localNamer: new JS.TemporaryNamer(jsTree)));
+  List<int> _summarizeModule(List<CompilationUnit> compilationUnits) {
+    if (!options.summarizeApi) return null;
 
-    if (options.sourceMap && options.sourceMapComment) {
-      printer.emit('\n//# sourceMappingURL=${unit.name}.js.map\n');
-    }
-
-    return new Tuple2(printer.getText(), sourceMap?.build(unit.name + '.js'));
+    var assembler = new PackageBundleAssembler();
+    compilationUnits
+        .map((u) => u.element.library)
+        .toSet()
+        .forEach(assembler.serializeLibraryElement);
+    return assembler.assemble().toBuffer();
   }
 
   JS.Program _emitModule(List<CompilationUnit> compilationUnits) {
@@ -284,18 +256,7 @@ class CodeGenerator extends GeneralizingAstVisitor
     _copyAndFlattenBlocks(items, _moduleItems);
 
     // Build the module.
-    var module = new JS.Program(items, name: _buildUnit.name);
-
-    // Optional: lower module format. Otherwise just return it.
-    switch (options.moduleFormat) {
-      case ModuleFormat.legacy:
-        return new LegacyModuleBuilder().build(module);
-      case ModuleFormat.node:
-        return new NodeModuleBuilder().build(module);
-      case ModuleFormat.es6:
-        return module;
-    }
-    return null; // unreachable. It is here to suppress a bogus Analyzer message
+    return new JS.Program(items, name: _buildUnit.name);
   }
 
   List<String> _getJSName(Element e) {
@@ -1750,6 +1711,7 @@ class CodeGenerator extends GeneralizingAstVisitor
       var e = js.call('() => #', o);
       return new JS.Property(_propertyName(name), e);
     }
+
     var sigFields = <JS.Property>[];
     if (!tCtors.isEmpty) sigFields.add(build('constructors', tCtors));
     if (!tMethods.isEmpty) sigFields.add(build('methods', tMethods));
@@ -3262,8 +3224,12 @@ class CodeGenerator extends GeneralizingAstVisitor
         var vars = <JS.MetaLetVariable, JS.Expression>{};
         var l = _visit(_bindValue(vars, 'l', target));
         jsTarget = new JS.MetaLet(vars, [
-          js.call('(#[(#[dart._extensionType]) ? dartx[#] : #])',
-              [l, l, memberName, memberName,])
+          js.call('(#[(#[dart._extensionType]) ? dartx[#] : #])', [
+            l,
+            l,
+            memberName,
+            memberName,
+          ])
         ]);
         if (typeArgs != null) jsTarget = new JS.Call(jsTarget, typeArgs);
         return new JS.Call(jsTarget, args);
@@ -3806,6 +3772,7 @@ class CodeGenerator extends GeneralizingAstVisitor
       var args = _visit(argumentList) as List<JS.Expression>;
       return isFactory ? new JS.Call(ctor, args) : new JS.New(ctor, args);
     }
+
     if (element != null && _isObjectLiteral(element.enclosingElement)) {
       return _emitObjectLiteral(argumentList);
     }
@@ -4047,6 +4014,7 @@ class CodeGenerator extends GeneralizingAstVisitor
       }
       return null;
     }
+
     if (expr is SimpleIdentifier) {
       return finishIdentifier(expr);
     } else if (expr is PrefixedIdentifier && !expr.isDeferred) {
@@ -4134,6 +4102,7 @@ class CodeGenerator extends GeneralizingAstVisitor
       if (value != null) return value.bitLength;
       return MAX;
     }
+
     return bitWidth(expr, 0) < 32;
   }
 
@@ -4924,6 +4893,7 @@ class CodeGenerator extends GeneralizingAstVisitor
       var name = js.string(node.components.join('.'), "'");
       return js.call('#.new(#)', [_emitType(types.symbolType), name]);
     }
+
     return _emitConst(emitSymbol);
   }
 
@@ -4952,6 +4922,7 @@ class CodeGenerator extends GeneralizingAstVisitor
       }
       return list;
     }
+
     if (isConst) return _cacheConst(emitList);
     return emitList();
   }
@@ -4990,6 +4961,7 @@ class CodeGenerator extends GeneralizingAstVisitor
       }
       return js.call('dart.map(#, #)', [mapArguments, types]);
     }
+
     if (node.constKeyword != null) return _emitConst(emitMap);
     return emitMap();
   }
@@ -5066,6 +5038,7 @@ class CodeGenerator extends GeneralizingAstVisitor
     JS.Expression finish(JS.Expression result) {
       return annotate(result, node);
     }
+
     if (node is PrefixExpression && node.operator.lexeme == '!') {
       return finish(js.call('!#', _visitTest(node.operand)));
     }
@@ -5077,6 +5050,7 @@ class CodeGenerator extends GeneralizingAstVisitor
         return finish(js.call(code,
             [_visitTest(node.leftOperand), _visitTest(node.rightOperand)]));
       }
+
       var op = node.operator.type.lexeme;
       if (op == '&&') return shortCircuit('# && #');
       if (op == '||') return shortCircuit('# || #');
@@ -5141,7 +5115,7 @@ class CodeGenerator extends GeneralizingAstVisitor
   ///     x.get('hi')
   ///     x.set('hi', 123)
   ///
-  /// This follows the same pattern as EcmaScript 6 Map:
+  /// This follows the same pattern as ECMAScript 6 Map:
   /// <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map>
   ///
   /// Unary minus looks like: `x['unary-']()`. Note that [unary] must be passed
@@ -5294,8 +5268,16 @@ class CodeGenerator extends GeneralizingAstVisitor
   /// within the file.
   ///
   /// If the value is null, the entire file is whitelisted.
+  ///
+  // TODO(jmesserly): why is this here, and what can we do to remove it?
+  //
+  // Hard coded lists are completely unnecessary -- if a feature is needed,
+  // metadata, type system features, or command line options are the right way
+  // to express it.
+  //
+  // As it is this is completely unsound and unmaintainable.
   static Map<String, List<String>> _uncheckedWhitelist = {
-    'dom_renderer.dart': ['moveNodesAfterSibling',],
+    'dom_renderer.dart': ['moveNodesAfterSibling'],
     'template_ref.dart': ['createEmbeddedView'],
     'ng_class.dart': ['_applyIterableChanges'],
     'ng_for.dart': ['_bulkRemove', '_bulkInsert'],
@@ -5335,7 +5317,8 @@ class CodeGenerator extends GeneralizingAstVisitor
   }
 }
 
-/// Choose a canonical name from the library element.
+/// Choose a canonical name from the [library] element.
+///
 /// This never uses the library's name (the identifier in the `library`
 /// declaration) as it doesn't have any meaningful rules enforced.
 String jsLibraryName(String libraryRoot, LibraryElement library) {

pkg/dev_compiler/lib/runtime/run.js renamed to pkg/dev_compiler/lib/js/common/run.js

@@ -2,7 +2,6 @@
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
-import 'dart:convert' show JSON;
 import 'dart:io';
 import 'package:analyzer/src/generated/source.dart' show Source;
 import 'package:analyzer/src/summary/package_bundle_reader.dart'
@@ -11,14 +10,16 @@ import 'package:args/args.dart' show ArgParser, ArgResults;
 import 'package:args/command_runner.dart' show UsageException;
 import 'package:path/path.dart' as path;
 
+import '../analyzer/context.dart' show AnalyzerOptions;
 import 'compiler.dart'
     show BuildUnit, CompilerOptions, JSModuleFile, ModuleCompiler;
-import '../analyzer/context.dart' show AnalyzerOptions;
+import 'module_builder.dart';
 
 final ArgParser _argParser = () {
   var argParser = new ArgParser()
     ..addFlag('help', abbr: 'h', help: 'Display this message.')
-    ..addOption('out', abbr: 'o', help: 'Output file (required).')
+    ..addOption('out',
+        abbr: 'o', allowMultiple: true, help: 'Output file (required).')
     ..addOption('module-root',
         help: 'Root module directory.\n'
             'Generated module paths are relative to this root.')
@@ -27,6 +28,7 @@ final ArgParser _argParser = () {
             'Generated library names are relative to this root.')
     ..addOption('build-root',
         help: 'Deprecated in favor of --library-root', hide: true);
+  addModuleFormatOptions(argParser, allowMultiple: true);
   AnalyzerOptions.addArguments(argParser);
   CompilerOptions.addArguments(argParser);
   return argParser;
@@ -88,13 +90,21 @@ void _compile(ArgResults argResults, void printFn(Object obj)) {
     printFn(_usageMessage);
     return;
   }
-  var outPath = argResults['out'];
+  var outPaths = argResults['out'] as List<String>;
+  var moduleFormats = parseModuleFormatOption(argResults);
 
-  if (outPath == null) {
+  if (outPaths.isEmpty) {
     _usageException('Please include the output file location. For example:\n'
         '    -o PATH/TO/OUTPUT_FILE.js');
+  } else if (outPaths.length != moduleFormats.length) {
+    _usageException('Number of output files (${outPaths.length}) must match '
+        'number of module formats (${moduleFormats.length}).');
   }
 
+  // TODO(jmesserly): for now the first one is special. This will go away once
+  // we've removed the "root" and "module name" variables.
+  var outPath = outPaths[0];
+
   var libraryRoot = argResults['library-root'] as String;
   libraryRoot ??= argResults['build-root'] as String;
   if (libraryRoot != null) {
@@ -126,17 +136,15 @@ void _compile(ArgResults argResults, void printFn(Object obj)) {
   if (!module.isValid) throw new CompileErrorException();
 
   // Write JS file, as well as source map and summary (if requested).
-  new File(outPath).writeAsStringSync(module.code);
-  if (module.sourceMap != null) {
-    var mapPath = outPath + '.map';
-    new File(mapPath)
-        .writeAsStringSync(JSON.encode(module.placeSourceMap(mapPath)));
-  }
-  if (module.summaryBytes != null) {
-    var summaryPath =
-        path.withoutExtension(outPath) + '.${compilerOpts.summaryExtension}';
-    new File(summaryPath).writeAsBytesSync(module.summaryBytes);
+  for (var i = 0; i < outPaths.length; i++) {
+    module.writeCodeSync(moduleFormats[i], outPaths[i]);
+    if (module.summaryBytes != null) {
+      var summaryPath =
+          path.withoutExtension(outPath) + '.${compilerOpts.summaryExtension}';
+      new File(summaryPath).writeAsBytesSync(module.summaryBytes);
+    }
   }
+
 }
 
 String _moduleForLibrary(