fix: put all ngc files into a single directory (angular#10486)

mhevery · web-flow · commit 790362e24391 · 2016-08-03T21:34:03.000-07:00
Prior to this change `ngc` would place generated files which refer to components in the node_modules into the node_module. This is an issue. Now all of the files are forced into a single directory as specified in `tsconfig.json` by the `genDir` option. see: https://docs.google.com/document/d/1OgP1RIpZ-lWUc4113J3w13HTDcW-1-0o7TuGz0tGx0g
diff --git a/modules/@angular/compiler-cli/src/codegen.ts b/modules/@angular/compiler-cli/src/codegen.ts
@@ -80,7 +80,14 @@ export class CodeGenerator {
       }
     }
 
-    return path.join(this.options.genDir, path.relative(root, filePath));
+    // transplant the codegen path to be inside the `genDir`
+    var relativePath: string = path.relative(root, filePath);
+    while (relativePath.startsWith('..' + path.sep)) {
+      // Strip out any `..` path such as: `../node_modules/@foo` as we want to put everything
+      // into `genDir`.
+      relativePath = relativePath.substr(3);
+    }
+    return path.join(this.options.genDir, relativePath);
   }
 
   codegen(): Promise<any> {
diff --git a/modules/@angular/compiler-cli/src/reflector_host.ts b/modules/@angular/compiler-cli/src/reflector_host.ts
@@ -16,6 +16,8 @@ import {StaticReflectorHost, StaticSymbol} from './static_reflector';
 
 const EXT = /(\.ts|\.d\.ts|\.js|\.jsx|\.tsx)$/;
 const DTS = /\.d\.ts$/;
+const NODE_MODULES = path.sep + 'node_modules' + path.sep;
+const IS_GENERATED = /\.(ngfactory|css(\.shim)?)$/;
 
 export interface ReflectorHostContext {
   fileExists(fileName: string): boolean;
@@ -27,10 +29,13 @@ export interface ReflectorHostContext {
 export class ReflectorHost implements StaticReflectorHost, ImportGenerator {
   private metadataCollector = new MetadataCollector();
   private context: ReflectorHostContext;
+  private isGenDirChildOfRootDir: boolean;
   constructor(
       private program: ts.Program, private compilerHost: ts.CompilerHost,
       private options: AngularCompilerOptions, context?: ReflectorHostContext) {
     this.context = context || new NodeReflectorHostContext();
+    var genPath: string = path.relative(options.basePath, options.genDir);
+    this.isGenDirChildOfRootDir = genPath === '' || !genPath.startsWith('..');
   }
 
   angularImportLocations() {
@@ -66,10 +71,19 @@ export class ReflectorHost implements StaticReflectorHost, ImportGenerator {
   /**
    * We want a moduleId that will appear in import statements in the generated code.
    * These need to be in a form that system.js can load, so absolute file paths don't work.
-   * Relativize the paths by checking candidate prefixes of the absolute path, to see if
-   * they are resolvable by the moduleResolution strategy from the CompilerHost.
+   *
+   * The `containingFile` is always in the `genDir`, where as the `importedFile` can be in
+   * `genDir`, `node_module` or `basePath`.  The `importedFile` is either a generated file or
+   * existing file.
+   *
+   *               | genDir   | node_module |  rootDir
+   * --------------+----------+-------------+----------
+   * generated     | relative |   relative  |   n/a
+   * existing file |   n/a    |   absolute  |  relative(*)
+   *
+   * NOTE: (*) the relative path is computed depending on `isGenDirChildOfRootDir`.
    */
-  getImportPath(containingFile: string, importedFile: string) {
+  getImportPath(containingFile: string, importedFile: string): string {
     importedFile = this.resolveAssetUrl(importedFile, containingFile);
     containingFile = this.resolveAssetUrl(containingFile, '');
 
@@ -79,27 +93,59 @@ export class ReflectorHost implements StaticReflectorHost, ImportGenerator {
       this.context.assumeFileExists(importedFile);
     }
 
-    const importModuleName = importedFile.replace(EXT, '');
-    const parts = importModuleName.split(path.sep).filter(p => !!p);
-
-    for (let index = parts.length - 1; index >= 0; index--) {
-      let candidate = parts.slice(index, parts.length).join(path.sep);
-      if (this.resolve('.' + path.sep + candidate, containingFile) === importedFile) {
-        return `./${candidate}`;
+    containingFile = this.rewriteGenDirPath(containingFile);
+    const containingDir = path.dirname(containingFile);
+    // drop extension
+    importedFile = importedFile.replace(EXT, '');
+
+    var nodeModulesIndex = importedFile.indexOf(NODE_MODULES);
+    const importModule = nodeModulesIndex === -1 ?
+        null :
+        importedFile.substring(nodeModulesIndex + NODE_MODULES.length);
+    const isGeneratedFile = IS_GENERATED.test(importedFile);
+
+    if (isGeneratedFile) {
+      // rewrite to genDir path
+      if (importModule) {
+        // it is generated, therefore we do a relative path to the factory
+        return this.dotRelative(containingDir, this.options.genDir + NODE_MODULES + importModule);
+      } else {
+        // assume that import is also in `genDir`
+        importedFile = this.rewriteGenDirPath(importedFile);
+        return this.dotRelative(containingDir, importedFile);
       }
-      if (this.resolve(candidate, containingFile) === importedFile) {
-        return candidate;
+    } else {
+      // user code import
+      if (importModule) {
+        return importModule;
+      } else {
+        if (!this.isGenDirChildOfRootDir) {
+          // assume that they are on top of each other.
+          importedFile = importedFile.replace(this.options.basePath, this.options.genDir);
+        }
+        return this.dotRelative(containingDir, importedFile);
       }
     }
+  }
 
-    // Try a relative import
-    let candidate = path.relative(path.dirname(containingFile), importModuleName);
-    if (this.resolve(candidate, containingFile) === importedFile) {
-      return candidate;
-    }
+  private dotRelative(from: string, to: string): string {
+    var rPath: string = path.relative(from, to);
+    return rPath.startsWith('.') ? rPath : './' + rPath;
+  }
 
-    throw new Error(
-        `Unable to find any resolvable import for ${importedFile} relative to ${containingFile}`);
+  /**
+   * Moves the path into `genDir` folder while preserving the `node_modules` directory.
+   */
+  private rewriteGenDirPath(filepath: string) {
+    var nodeModulesIndex = filepath.indexOf(NODE_MODULES);
+    if (nodeModulesIndex !== -1) {
+      // If we are in node_modulse, transplant them into `genDir`.
+      return path.join(this.options.genDir, filepath.substring(nodeModulesIndex));
+    } else {
+      // pretend that containing file is on top of the `genDir` to normalize the paths.
+      // we apply the `genDir` => `rootDir` delta through `rootDirPrefix` later.
+      return filepath.replace(this.options.basePath, this.options.genDir);
+    }
   }
 
   findDeclaration(
diff --git a/modules/@angular/compiler-cli/test/reflector_host_spec.ts b/modules/@angular/compiler-cli/test/reflector_host_spec.ts
@@ -17,7 +17,8 @@ describe('reflector_host', () => {
   var context: MockContext;
   var host: ts.CompilerHost;
   var program: ts.Program;
-  var reflectorHost: ReflectorHost;
+  var reflectorNestedGenDir: ReflectorHost;
+  var reflectorSiblingGenDir: ReflectorHost;
 
   beforeEach(() => {
     context = new MockContext('/tmp/src', clone(FILES));
@@ -32,20 +33,95 @@ describe('reflector_host', () => {
     if (errors && errors.length) {
       throw new Error('Expected no errors');
     }
-    reflectorHost = new ReflectorHost(
+    reflectorNestedGenDir = new ReflectorHost(
         program, host, {
-          genDir: '/tmp/dist',
-          basePath: '/tmp/src',
+          genDir: '/tmp/project/src/gen',
+          basePath: '/tmp/project/src',
           skipMetadataEmit: false,
           skipTemplateCodegen: false,
           trace: false
         },
         context);
+    reflectorSiblingGenDir = new ReflectorHost(
+        program, host, {
+          genDir: '/tmp/project/gen',
+          basePath: '/tmp/project/src',
+          skipMetadataEmit: false,
+          skipTemplateCodegen: false,
+          trace: false
+        },
+        context);
+  });
+
+  describe('nestedGenDir', () => {
+    it('should import node_module from factory', () => {
+      expect(reflectorNestedGenDir.getImportPath(
+                 '/tmp/project/src/gen/my.ngfactory.ts',
+                 '/tmp/project/node_modules/@angular/core.d.ts'))
+          .toEqual('@angular/core');
+    });
+
+    it('should import factory from factory', () => {
+      expect(reflectorNestedGenDir.getImportPath(
+                 '/tmp/project/src/my.ngfactory.ts', '/tmp/project/src/my.other.ngfactory.ts'))
+          .toEqual('./my.other.ngfactory');
+      expect(reflectorNestedGenDir.getImportPath(
+                 '/tmp/project/src/a/my.ngfactory.ts', '/tmp/project/src/my.other.css.ts'))
+          .toEqual('../my.other.css');
+      expect(reflectorNestedGenDir.getImportPath(
+                 '/tmp/project/src/my.ngfactory.ts', '/tmp/project/src/a/my.other.css.shim.ts'))
+          .toEqual('./a/my.other.css.shim');
+    });
+
+    it('should import application from factory', () => {
+      expect(reflectorNestedGenDir.getImportPath(
+                 '/tmp/project/src/my.ngfactory.ts', '/tmp/project/src/my.other.ts'))
+          .toEqual('../my.other');
+      expect(reflectorNestedGenDir.getImportPath(
+                 '/tmp/project/src/a/my.ngfactory.ts', '/tmp/project/src/my.other.ts'))
+          .toEqual('../../my.other');
+      expect(reflectorNestedGenDir.getImportPath(
+                 '/tmp/project/src/my.ngfactory.ts', '/tmp/project/src/a/my.other.ts'))
+          .toEqual('../a/my.other');
+    });
+  });
+
+  describe('nestedGenDir', () => {
+    it('should import node_module from factory', () => {
+      expect(reflectorSiblingGenDir.getImportPath(
+                 '/tmp/project/src/gen/my.ngfactory.ts',
+                 '/tmp/project/node_modules/@angular/core.d.ts'))
+          .toEqual('@angular/core');
+    });
+
+    it('should import factory from factory', () => {
+      expect(reflectorSiblingGenDir.getImportPath(
+                 '/tmp/project/src/my.ngfactory.ts', '/tmp/project/src/my.other.ngfactory.ts'))
+          .toEqual('./my.other.ngfactory');
+      expect(reflectorSiblingGenDir.getImportPath(
+                 '/tmp/project/src/a/my.ngfactory.ts', '/tmp/project/src/my.other.css.ts'))
+          .toEqual('../my.other.css');
+      expect(reflectorSiblingGenDir.getImportPath(
+                 '/tmp/project/src/my.ngfactory.ts', '/tmp/project/src/a/my.other.css.shim.ts'))
+          .toEqual('./a/my.other.css.shim');
+    });
+
+    it('should import application from factory', () => {
+      expect(reflectorSiblingGenDir.getImportPath(
+                 '/tmp/project/src/my.ngfactory.ts', '/tmp/project/src/my.other.ts'))
+          .toEqual('./my.other');
+      expect(reflectorSiblingGenDir.getImportPath(
+                 '/tmp/project/src/a/my.ngfactory.ts', '/tmp/project/src/my.other.ts'))
+          .toEqual('../my.other');
+      expect(reflectorSiblingGenDir.getImportPath(
+                 '/tmp/project/src/my.ngfactory.ts', '/tmp/project/src/a/my.other.ts'))
+          .toEqual('./a/my.other');
+    });
   });
 
   it('should provide the import locations for angular', () => {
     let {coreDecorators, diDecorators, diMetadata, animationMetadata, provider} =
-        reflectorHost.angularImportLocations();
+        reflectorNestedGenDir.angularImportLocations();
     expect(coreDecorators).toEqual('@angular/core/src/metadata');
     expect(diDecorators).toEqual('@angular/core/src/di/decorators');
     expect(diMetadata).toEqual('@angular/core/src/di/metadata');
@@ -54,82 +130,86 @@ describe('reflector_host', () => {
   });
 
   it('should be able to produce an import from main @angular/core', () => {
-    expect(reflectorHost.getImportPath('main.ts', 'node_modules/@angular/core.d.ts'))
+    expect(reflectorNestedGenDir.getImportPath(
+               '/tmp/project/src/main.ts', '/tmp/project/node_modules/@angular/core.d.ts'))
         .toEqual('@angular/core');
   });
 
-  it('should be ble to produce an import from main to a sub-directory', () => {
-    expect(reflectorHost.getImportPath('main.ts', 'lib/utils.ts')).toEqual('./lib/utils');
+  it('should be able to produce an import from main to a sub-directory', () => {
+    expect(reflectorNestedGenDir.getImportPath('main.ts', 'lib/utils.ts')).toEqual('./lib/utils');
   });
 
   it('should be able to produce an import from to a peer file', () => {
-    expect(reflectorHost.getImportPath('lib/utils.ts', 'lib/collections.ts'))
+    expect(reflectorNestedGenDir.getImportPath('lib/utils.ts', 'lib/collections.ts'))
         .toEqual('./collections');
   });
 
   it('should be able to produce an import from to a sibling directory', () => {
-    expect(reflectorHost.getImportPath('lib2/utils2.ts', 'lib/utils.ts')).toEqual('../lib/utils');
+    expect(reflectorNestedGenDir.getImportPath('lib2/utils2.ts', 'lib/utils.ts'))
+        .toEqual('../lib/utils');
   });
 
   it('should be able to produce a symbol for an exported symbol', () => {
-    expect(reflectorHost.findDeclaration('@angular/router-deprecated', 'foo', 'main.ts'))
+    expect(reflectorNestedGenDir.findDeclaration('@angular/router-deprecated', 'foo', 'main.ts'))
         .toBeDefined();
   });
 
   it('should be able to produce a symbol for values space only reference', () => {
-    expect(
-        reflectorHost.findDeclaration('@angular/router-deprecated/src/providers', 'foo', 'main.ts'))
+    expect(reflectorNestedGenDir.findDeclaration(
+               '@angular/router-deprecated/src/providers', 'foo', 'main.ts'))
         .toBeDefined();
   });
 
 
   it('should be produce the same symbol if asked twice', () => {
-    let foo1 = reflectorHost.getStaticSymbol('main.ts', 'foo');
-    let foo2 = reflectorHost.getStaticSymbol('main.ts', 'foo');
+    let foo1 = reflectorNestedGenDir.getStaticSymbol('main.ts', 'foo');
+    let foo2 = reflectorNestedGenDir.getStaticSymbol('main.ts', 'foo');
     expect(foo1).toBe(foo2);
   });
 
   it('should be able to produce a symbol for a module with no file', () => {
-    expect(reflectorHost.getStaticSymbol('angularjs', 'SomeAngularSymbol')).toBeDefined();
+    expect(reflectorNestedGenDir.getStaticSymbol('angularjs', 'SomeAngularSymbol')).toBeDefined();
   });
 
   it('should be able to read a metadata file', () => {
-    expect(reflectorHost.getMetadataFor('node_modules/@angular/core.d.ts'))
+    expect(reflectorNestedGenDir.getMetadataFor('node_modules/@angular/core.d.ts'))
         .toEqual({__symbolic: 'module', version: 1, metadata: {foo: {__symbolic: 'class'}}});
   });
 
   it('should be able to read metadata from an otherwise unused .d.ts file ', () => {
-    expect(reflectorHost.getMetadataFor('node_modules/@angular/unused.d.ts')).toBeUndefined();
+    expect(reflectorNestedGenDir.getMetadataFor('node_modules/@angular/unused.d.ts'))
+        .toBeUndefined();
   });
 
   it('should return undefined for missing modules', () => {
-    expect(reflectorHost.getMetadataFor('node_modules/@angular/missing.d.ts')).toBeUndefined();
+    expect(reflectorNestedGenDir.getMetadataFor('node_modules/@angular/missing.d.ts'))
+        .toBeUndefined();
   });
 
   it('should be able to trace a named export', () => {
-    const symbol =
-        reflectorHost.findDeclaration('./reexport/reexport.d.ts', 'One', '/tmp/src/main.ts');
+    const symbol = reflectorNestedGenDir.findDeclaration(
+        './reexport/reexport.d.ts', 'One', '/tmp/src/main.ts');
     expect(symbol.name).toEqual('One');
     expect(symbol.filePath).toEqual('/tmp/src/reexport/src/origin1.d.ts');
   });
 
   it('should be able to trace a renamed export', () => {
-    const symbol =
-        reflectorHost.findDeclaration('./reexport/reexport.d.ts', 'Four', '/tmp/src/main.ts');
+    const symbol = reflectorNestedGenDir.findDeclaration(
+        './reexport/reexport.d.ts', 'Four', '/tmp/src/main.ts');
     expect(symbol.name).toEqual('Three');
     expect(symbol.filePath).toEqual('/tmp/src/reexport/src/origin1.d.ts');
   });
 
   it('should be able to trace an export * export', () => {
-    const symbol =
-        reflectorHost.findDeclaration('./reexport/reexport.d.ts', 'Five', '/tmp/src/main.ts');
+    const symbol = reflectorNestedGenDir.findDeclaration(
+        './reexport/reexport.d.ts', 'Five', '/tmp/src/main.ts');
     expect(symbol.name).toEqual('Five');
     expect(symbol.filePath).toEqual('/tmp/src/reexport/src/origin5.d.ts');
   });
 
   it('should be able to trace a multi-level re-export', () => {
-    const symbol =
-        reflectorHost.findDeclaration('./reexport/reexport.d.ts', 'Thirty', '/tmp/src/main.ts');
+    const symbol = reflectorNestedGenDir.findDeclaration(
+        './reexport/reexport.d.ts', 'Thirty', '/tmp/src/main.ts');
     expect(symbol.name).toEqual('Thirty');
     expect(symbol.filePath).toEqual('/tmp/src/reexport/src/origin30.d.ts');
   });
diff --git a/modules/@angular/compiler/src/output/ts_emitter.ts b/modules/@angular/compiler/src/output/ts_emitter.ts
@@ -254,7 +254,7 @@ class _TsEmitterVisitor extends AbstractEmitterVisitor implements o.TypeVisitor
   }
 
   getBuiltinMethodName(method: o.BuiltinMethod): string {
-    var name: any /** TODO #9100 */;
+    var name: string;
     switch (method) {
       case o.BuiltinMethod.ConcatArray:
         name = 'concat';

Original file line number	Diff line number	Diff line change
`@@ -80,7 +80,14 @@ export class CodeGenerator {`
`80`	`80`	`}`
`81`	`81`	`}`
`82`	`82`
`83`		`- return path.join(this.options.genDir, path.relative(root, filePath));`
	`83`	+ // transplant the codegen path to be inside the `genDir`
	`84`	`+ var relativePath: string = path.relative(root, filePath);`
	`85`	`+ while (relativePath.startsWith('..' + path.sep)) {`
	`86`	+ // Strip out any `..` path such as: `../node_modules/@foo` as we want to put everything
	`87`	+ // into `genDir`.
	`88`	`+ relativePath = relativePath.substr(3);`
	`89`	`+ }`
	`90`	`+ return path.join(this.options.genDir, relativePath);`
`84`	`91`	`}`
`85`	`92`
`86`	`93`	`codegen(): Promise<any> {`
Original file line number	Diff line number	Diff line change
`@@ -254,7 +254,7 @@ class _TsEmitterVisitor extends AbstractEmitterVisitor implements o.TypeVisitor`
`254`	`254`	`}`
`255`	`255`
`256`	`256`	`getBuiltinMethodName(method: o.BuiltinMethod): string {`
`257`		`- var name: any /** TODO #9100 */;`
	`257`	`+ var name: string;`
`258`	`258`	`switch (method) {`
`259`	`259`	`case o.BuiltinMethod.ConcatArray:`
`260`	`260`	`name = 'concat';`