fix(bazel): do not use manifest paths for generated imports within compilation unit (#35841)

Currently, the `ng_module` rule incorrectly uses manifest paths for
generated imports from the Angular compiler.

This breaks packaging as prodmode output (i.e. `esnext`) is copied in
various targets (`es5` and `es2015`) to the npm package output.

e.g. imports are generated like:

_node_modules/my-pkg/es2015/imports/public-api.js_
```ts
import * as i1 from "angular/packages/bazel/test/ng_package/example/imports/second";
```

while it should be actually:

```ts
import * as i1 from "./second";
```

The imports can, and should be relative so that the files are
self-contained and do not rely on custom module resolution.

PR Close #35841
This commit is contained in:
Paul Gschwendtner 2020-03-05 20:42:58 +01:00 committed by Matias Niemelä
parent 68bebd67f7
commit 958165888c
3 changed files with 65 additions and 23 deletions

View File

@ -315,7 +315,12 @@ def _ngc_tsconfig(ctx, files, srcs, **kwargs):
"createExternalSymbolFactoryReexports": (not _is_bazel()), "createExternalSymbolFactoryReexports": (not _is_bazel()),
# FIXME: wrong place to de-dupe # FIXME: wrong place to de-dupe
"expectedOut": depset([o.path for o in expected_outs]).to_list(), "expectedOut": depset([o.path for o in expected_outs]).to_list(),
# We instruct the compiler to use the host for import generation in Blaze. By default,
# module names between source files of the same compilation unit are relative paths. This
# is not desired in google3 where the generated module names are used as qualified names
# for aliased exports. We disable relative paths and always use manifest paths in google3.
"_useHostForImportGeneration": (not _is_bazel()), "_useHostForImportGeneration": (not _is_bazel()),
"_useManifestPathsAsModuleName": (not _is_bazel()),
} }
if _should_produce_flat_module_outs(ctx): if _should_produce_flat_module_outs(ctx):

View File

@ -113,19 +113,17 @@ export function runOneBuild(args: string[], inputs?: {[path: string]: string}):
} }
} }
const expectedOuts = config['angularCompilerOptions']['expectedOut']; // These are options passed through from the `ng_module` rule which aren't supported
// by the `@angular/compiler-cli` and are only intended for `ngc-wrapped`.
const {expectedOut, _useManifestPathsAsModuleName} = config['angularCompilerOptions'];
const {basePath} = ng.calcProjectFileAndBasePath(project); const {basePath} = ng.calcProjectFileAndBasePath(project);
const compilerOpts = ng.createNgCompilerOptions(basePath, config, tsOptions); const compilerOpts = ng.createNgCompilerOptions(basePath, config, tsOptions);
const tsHost = ts.createCompilerHost(compilerOpts, true); const tsHost = ts.createCompilerHost(compilerOpts, true);
const {diagnostics} = compile({ const {diagnostics} = compile({
allDepsCompiledWithBazel: ALL_DEPS_COMPILED_WITH_BAZEL, allDepsCompiledWithBazel: ALL_DEPS_COMPILED_WITH_BAZEL,
compilerOpts, useManifestPathsAsModuleName: _useManifestPathsAsModuleName,
tsHost, expectedOuts: expectedOut, compilerOpts, tsHost, bazelOpts, files, inputs,
bazelOpts,
files,
inputs,
expectedOuts
}); });
if (diagnostics.length) { if (diagnostics.length) {
console.error(ng.formatDiagnostics(diagnostics)); console.error(ng.formatDiagnostics(diagnostics));
@ -144,9 +142,11 @@ export function relativeToRootDirs(filePath: string, rootDirs: string[]): string
return filePath; return filePath;
} }
export function compile({allDepsCompiledWithBazel = true, compilerOpts, tsHost, bazelOpts, files, export function compile({allDepsCompiledWithBazel = true, useManifestPathsAsModuleName,
inputs, expectedOuts, gatherDiagnostics, bazelHost}: { compilerOpts, tsHost, bazelOpts, files, inputs, expectedOuts,
gatherDiagnostics, bazelHost}: {
allDepsCompiledWithBazel?: boolean, allDepsCompiledWithBazel?: boolean,
useManifestPathsAsModuleName?: boolean,
compilerOpts: ng.CompilerOptions, compilerOpts: ng.CompilerOptions,
tsHost: ts.CompilerHost, inputs?: {[path: string]: string}, tsHost: ts.CompilerHost, inputs?: {[path: string]: string},
bazelOpts: BazelOptions, bazelOpts: BazelOptions,
@ -199,13 +199,14 @@ export function compile({allDepsCompiledWithBazel = true, compilerOpts, tsHost,
throw new Error(`Couldn't find bazel bin in the rootDirs: ${compilerOpts.rootDirs}`); throw new Error(`Couldn't find bazel bin in the rootDirs: ${compilerOpts.rootDirs}`);
} }
const expectedOutsSet = new Set(expectedOuts.map(p => p.replace(/\\/g, '/'))); const expectedOutsSet = new Set(expectedOuts.map(p => convertToForwardSlashPath(p)));
const originalWriteFile = tsHost.writeFile.bind(tsHost); const originalWriteFile = tsHost.writeFile.bind(tsHost);
tsHost.writeFile = tsHost.writeFile =
(fileName: string, content: string, writeByteOrderMark: boolean, (fileName: string, content: string, writeByteOrderMark: boolean,
onError?: (message: string) => void, sourceFiles?: ts.SourceFile[]) => { onError?: (message: string) => void, sourceFiles?: ts.SourceFile[]) => {
const relative = relativeToRootDirs(fileName.replace(/\\/g, '/'), [compilerOpts.rootDir]); const relative =
relativeToRootDirs(convertToForwardSlashPath(fileName), [compilerOpts.rootDir]);
if (expectedOutsSet.has(relative)) { if (expectedOutsSet.has(relative)) {
expectedOutsSet.delete(relative); expectedOutsSet.delete(relative);
originalWriteFile(fileName, content, writeByteOrderMark, onError, sourceFiles); originalWriteFile(fileName, content, writeByteOrderMark, onError, sourceFiles);
@ -290,20 +291,32 @@ export function compile({allDepsCompiledWithBazel = true, compilerOpts, tsHost,
const ngHost = ng.createCompilerHost({options: compilerOpts, tsHost: bazelHost}); const ngHost = ng.createCompilerHost({options: compilerOpts, tsHost: bazelHost});
const fileNameToModuleNameCache = new Map<string, string>(); const fileNameToModuleNameCache = new Map<string, string>();
ngHost.fileNameToModuleName = (importedFilePath: string, containingFilePath: string) => { ngHost.fileNameToModuleName = (importedFilePath: string, containingFilePath?: string) => {
const cacheKey = `${importedFilePath}:${containingFilePath}`;
// Memoize this lookup to avoid expensive re-parses of the same file // Memoize this lookup to avoid expensive re-parses of the same file
// When run as a worker, the actual ts.SourceFile is cached // When run as a worker, the actual ts.SourceFile is cached
// but when we don't run as a worker, there is no cache. // but when we don't run as a worker, there is no cache.
// For one example target in g3, we saw a cache hit rate of 7590/7695 // For one example target in g3, we saw a cache hit rate of 7590/7695
if (fileNameToModuleNameCache.has(importedFilePath)) { if (fileNameToModuleNameCache.has(cacheKey)) {
return fileNameToModuleNameCache.get(importedFilePath); return fileNameToModuleNameCache.get(cacheKey);
} }
const result = doFileNameToModuleName(importedFilePath); const result = doFileNameToModuleName(importedFilePath, containingFilePath);
fileNameToModuleNameCache.set(importedFilePath, result); fileNameToModuleNameCache.set(cacheKey, result);
return result; return result;
}; };
function doFileNameToModuleName(importedFilePath: string): string { function doFileNameToModuleName(importedFilePath: string, containingFilePath?: string): string {
const relativeTargetPath =
relativeToRootDirs(importedFilePath, compilerOpts.rootDirs).replace(EXT, '');
const manifestTargetPath = `${bazelOpts.workspaceName}/${relativeTargetPath}`;
if (useManifestPathsAsModuleName === true) {
return manifestTargetPath;
}
// Unless manifest paths are explicitly enforced, we initially check if a module name is
// set for the given source file. The compiler host from `@bazel/typescript` sets source
// file module names if the compilation targets either UMD or AMD. To ensure that the AMD
// module names match, we first consider those.
try { try {
const sourceFile = ngHost.getSourceFile(importedFilePath, ts.ScriptTarget.Latest); const sourceFile = ngHost.getSourceFile(importedFilePath, ts.ScriptTarget.Latest);
if (sourceFile && sourceFile.moduleName) { if (sourceFile && sourceFile.moduleName) {
@ -342,11 +355,31 @@ export function compile({allDepsCompiledWithBazel = true, compilerOpts, tsHost,
ngHost.amdModuleName) { ngHost.amdModuleName) {
return ngHost.amdModuleName({ fileName: importedFilePath } as ts.SourceFile); return ngHost.amdModuleName({ fileName: importedFilePath } as ts.SourceFile);
} }
const result = relativeToRootDirs(importedFilePath, compilerOpts.rootDirs).replace(EXT, '');
if (result.startsWith(NODE_MODULES)) { // If no AMD module name has been set for the source file by the `@bazel/typescript` compiler
return result.substr(NODE_MODULES.length); // host, and the target file is not part of a flat module node module package, we use the
// following rules (in order):
// 1. If target file is part of `node_modules/`, we use the package module name.
// 2. If no containing file is specified, or the target file is part of a different
// compilation unit, we use a Bazel manifest path. Relative paths are not possible
// since we don't have a containing file, and the target file could be located in the
// output directory, or in an external Bazel repository.
// 3. If both rules above didn't match, we compute a relative path between the source files
// since they are part of the same compilation unit.
// Note that we don't want to always use (2) because it could mean that compilation outputs
// are always leaking Bazel-specific paths, and the output is not self-contained. This could
// break `esm2015` or `esm5` output for Angular package release output
// Omit the `node_modules` prefix if the module name of an NPM package is requested.
if (relativeTargetPath.startsWith(NODE_MODULES)) {
return relativeTargetPath.substr(NODE_MODULES.length);
} else if (
containingFilePath == null || !bazelOpts.compilationTargetSrc.includes(importedFilePath)) {
return manifestTargetPath;
} }
return bazelOpts.workspaceName + '/' + result; const containingFileDir =
path.dirname(relativeToRootDirs(containingFilePath, compilerOpts.rootDirs));
const relativeImportPath = path.posix.relative(containingFileDir, relativeTargetPath);
return relativeImportPath.startsWith('.') ? relativeImportPath : `./${relativeImportPath}`;
} }
ngHost.toSummaryFileName = (fileName: string, referringSrcFileName: string) => path.posix.join( ngHost.toSummaryFileName = (fileName: string, referringSrcFileName: string) => path.posix.join(
@ -464,6 +497,10 @@ function isCompilationTarget(bazelOpts: BazelOptions, sf: ts.SourceFile): boolea
(bazelOpts.compilationTargetSrc.indexOf(sf.fileName) !== -1); (bazelOpts.compilationTargetSrc.indexOf(sf.fileName) !== -1);
} }
function convertToForwardSlashPath(filePath: string): string {
return filePath.replace(/\\/g, '/');
}
function gatherDiagnosticsForInputsOnly( function gatherDiagnosticsForInputsOnly(
options: ng.CompilerOptions, bazelOpts: BazelOptions, options: ng.CompilerOptions, bazelOpts: BazelOptions,
ngProgram: ng.Program): (ng.Diagnostic | ts.Diagnostic)[] { ngProgram: ng.Program): (ng.Diagnostic | ts.Diagnostic)[] {

View File

@ -1455,7 +1455,7 @@ export { MyService } from './public-api';
import { Injectable } from '@angular/core'; import { Injectable } from '@angular/core';
import { MySecondService } from './second'; import { MySecondService } from './second';
import * as i0 from "@angular/core"; import * as i0 from "@angular/core";
import * as i1 from "angular/packages/bazel/test/ng_package/example/imports/second"; import * as i1 from "./second";
export class MyService { export class MyService {
/** /**
* @param {?} secondService * @param {?} secondService
@ -1684,7 +1684,7 @@ import { __decorate, __metadata } from "tslib";
import { Injectable } from '@angular/core'; import { Injectable } from '@angular/core';
import { MySecondService } from './second'; import { MySecondService } from './second';
import * as i0 from "@angular/core"; import * as i0 from "@angular/core";
import * as i1 from "angular/packages/bazel/test/ng_package/example/imports/second"; import * as i1 from "./second";
var MyService = /** @class */ (function () { var MyService = /** @class */ (function () {
function MyService(secondService) { function MyService(secondService) {
this.secondService = secondService; this.secondService = secondService;