Skip to content

Commit

Permalink
Qualify library names in packages
Browse files Browse the repository at this point in the history
Partially addresses #504.  This is WIP, but wanted your thoughts.

Comment / questions:
- The current js_ast ImportDeclaration assumes a legal identifier I think.
  E.g., this gets lowered:
  import { src$interfaces } from "matcher"

- Should we thread package root through ModuleCompiler to let it packagify urls?

- How should we handle non-package urls?  Pass in an explicit root dir
  and go relative?

R=jmesserly@google.com

Review URL: https://codereview.chromium.org/1917863005 .
  • Loading branch information
vsmenon committed Apr 28, 2016
1 parent 9de1e74 commit 3483e19
Show file tree
Hide file tree
Showing 7 changed files with 948 additions and 887 deletions.
35 changes: 30 additions & 5 deletions pkg/dev_compiler/lib/src/compiler/code_generator.dart
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,8 @@ class CodeGenerator extends GeneralizingAstVisitor

BuildUnit _buildUnit;

String _buildRoot;

CodeGenerator(AnalysisContext c, this.options, this._extensionTypes)
: context = c,
types = c.typeProvider,
Expand All @@ -131,6 +133,10 @@ class CodeGenerator extends GeneralizingAstVisitor
JSModuleFile compile(BuildUnit unit, List<CompilationUnit> compilationUnits,
List<String> errors) {
_buildUnit = unit;
_buildRoot = _buildUnit.buildRoot;
if (!_buildRoot.endsWith('/')) {
_buildRoot = '$_buildRoot/';
}

var jsTree = _emitModule(compilationUnits);
var codeAndSourceMap = _writeJSText(unit, jsTree);
Expand Down Expand Up @@ -190,7 +196,7 @@ class CodeGenerator extends GeneralizingAstVisitor

var libraryTemp = _isDartRuntime(library)
? _runtimeLibVar
: new JS.TemporaryId(jsLibraryName(library));
: new JS.TemporaryId(jsLibraryName(_buildRoot, library));
_libraries[library] = libraryTemp;
items.add(new JS.ExportDeclaration(
js.call('const # = Object.create(null)', [libraryTemp])));
Expand Down Expand Up @@ -3760,8 +3766,8 @@ class CodeGenerator extends GeneralizingAstVisitor
JS.Identifier emitLibraryName(LibraryElement library) {
// It's either one of the libraries in this module, or it's an import.
return _libraries[library] ??
_imports.putIfAbsent(
library, () => new JS.TemporaryId(jsLibraryName(library)));
_imports.putIfAbsent(library,
() => new JS.TemporaryId(jsLibraryName(_buildRoot, library)));
}

JS.Node annotate(JS.Node node, AstNode original, [Element element]) {
Expand Down Expand Up @@ -3837,8 +3843,27 @@ class CodeGenerator extends GeneralizingAstVisitor
/// 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(LibraryElement library) {
return pathToJSIdentifier(library.source.uri.pathSegments.last);
String jsLibraryName(String buildRoot, LibraryElement library) {
var uri = library.source.uri;
if (uri.scheme == 'dart') {
return uri.path;
}
// TODO(vsm): This is not necessarily unique if '__' appears in a file name.
var separator = '__';
String qualifiedPath;
if (uri.scheme == 'package') {
// Strip the package name.
// TODO(vsm): This is not unique if an escaped '/'appears in a filename.
// E.g., "foo/bar.dart" and "foo$47bar.dart" would collide.
qualifiedPath = uri.pathSegments.skip(1).join(separator);
} else if (uri.path.startsWith(buildRoot)) {
qualifiedPath =
uri.path.substring(buildRoot.length).replaceAll('/', separator);
} else {
// We don't have a unique name.
throw 'Invalid build root. $buildRoot does not contain ${uri.path}';
}
return pathToJSIdentifier(qualifiedPath);
}

/// Shorthand for identifier-like property names.
Expand Down
13 changes: 12 additions & 1 deletion pkg/dev_compiler/lib/src/compiler/command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ class CompileCommand extends Command {
CompileCommand({MessageHandler messageHandler})
: this.messageHandler = messageHandler ?? print {
argParser.addOption('out', abbr: 'o', help: 'Output file (required)');
argParser.addOption('build-root',
help: '''
Root of source files. Generated library names are relative to this root.
''');
CompilerOptions.addArguments(argParser);
AnalyzerOptions.addArguments(argParser);
}
Expand All @@ -39,7 +43,14 @@ class CompileCommand extends Command {
usageException('Please include the output file location. For example:\n'
' -o PATH/TO/OUTPUT_FILE.js');
}
var unit = new BuildUnit(path.basenameWithoutExtension(outPath),

var buildRoot = argResults['build-root'] as String;
if (buildRoot != null) {
buildRoot = path.absolute(buildRoot);
} else {
buildRoot = Directory.current.path;
}
var unit = new BuildUnit(path.basenameWithoutExtension(outPath), buildRoot,
argResults.rest, _moduleForLibrary);

JSModuleFile module = compiler.compile(unit, compilerOptions);
Expand Down
5 changes: 4 additions & 1 deletion pkg/dev_compiler/lib/src/compiler/compiler.dart
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,9 @@ class BuildUnit {
/// The name of this module.
final String name;

/// Build root. All library names are relative to this path/prefix.
final String buildRoot;

/// The list of sources in this module.
///
/// The set of Dart files can be arbitrarily large, but it must contain
Expand All @@ -209,7 +212,7 @@ class BuildUnit {
// build units.
final Func1<Source, String> libraryToModule;

BuildUnit(this.name, this.sources, this.libraryToModule);
BuildUnit(this.name, this.buildRoot, this.sources, this.libraryToModule);
}

/// The output of Dart->JS compilation.
Expand Down
Loading

0 comments on commit 3483e19

Please sign in to comment.