fix(ivy): ngtsc is unable to detect flat module entry-point on windows (#29453)

Currently when building an Angular project with `ngtsc`
and `flatModuleOutFile` enabled, the Ngtsc build will fail
if there are multiple source files as root file names.

Ngtsc and NGC currently determine the entry-point for multiple
root file names by looking for files ending with `/index.ts`.

This functionality is technically deprecated, but still supported
and currently breaks on Windows as the root file names are not
guaranteed to be normalized POSIX-like paths.

In order to make this logic more reliable in the future, this commit
also switches the shim generators and entry-point logic to the branded
path types. This ensures that we don't break this in the future.

PR Close #29453
This commit is contained in:
Paul Gschwendtner 2019-03-26 23:39:12 +01:00 committed by Miško Hevery
parent e57ed61448
commit 1e5a818719
10 changed files with 67 additions and 37 deletions

View File

@ -10,6 +10,7 @@ ts_library(
module_name = "@angular/compiler-cli/src/ngtsc/entry_point", module_name = "@angular/compiler-cli/src/ngtsc/entry_point",
deps = [ deps = [
"//packages/compiler-cli/src/ngtsc/diagnostics", "//packages/compiler-cli/src/ngtsc/diagnostics",
"//packages/compiler-cli/src/ngtsc/path",
"//packages/compiler-cli/src/ngtsc/shims", "//packages/compiler-cli/src/ngtsc/shims",
"//packages/compiler-cli/src/ngtsc/util", "//packages/compiler-cli/src/ngtsc/util",
"@npm//@types/node", "@npm//@types/node",

View File

@ -6,10 +6,10 @@
* found in the LICENSE file at https://angular.io/license * found in the LICENSE file at https://angular.io/license
*/ */
import {normalizeSeparators} from '../../util/src/path'; import {AbsoluteFsPath} from '../../path/src/types';
import {isNonDeclarationTsPath} from '../../util/src/typescript'; import {isNonDeclarationTsPath} from '../../util/src/typescript';
export function findFlatIndexEntryPoint(rootFiles: ReadonlyArray<string>): string|null { export function findFlatIndexEntryPoint(rootFiles: ReadonlyArray<AbsoluteFsPath>): string|null {
// There are two ways for a file to be recognized as the flat module index: // There are two ways for a file to be recognized as the flat module index:
// 1) if it's the only file!!!!!! // 1) if it's the only file!!!!!!
// 2) (deprecated) if it's named 'index.ts' and has the shortest path of all such files. // 2) (deprecated) if it's named 'index.ts' and has the shortest path of all such files.
@ -33,5 +33,5 @@ export function findFlatIndexEntryPoint(rootFiles: ReadonlyArray<string>): strin
} }
} }
return resolvedEntryPoint ? normalizeSeparators(resolvedEntryPoint) : null; return resolvedEntryPoint;
} }

View File

@ -11,6 +11,7 @@ ts_library(
deps = [ deps = [
"//packages:types", "//packages:types",
"//packages/compiler-cli/src/ngtsc/entry_point", "//packages/compiler-cli/src/ngtsc/entry_point",
"//packages/compiler-cli/src/ngtsc/path",
"@npm//typescript", "@npm//typescript",
], ],
) )

View File

@ -7,22 +7,23 @@
* found in the LICENSE file at https://angular.io/license * found in the LICENSE file at https://angular.io/license
*/ */
import {AbsoluteFsPath} from '../../path/src/types';
import {findFlatIndexEntryPoint} from '../src/logic'; import {findFlatIndexEntryPoint} from '../src/logic';
describe('entry_point logic', () => { describe('entry_point logic', () => {
describe('findFlatIndexEntryPoint', () => { describe('findFlatIndexEntryPoint', () => {
it('should use the only source file if only a single one is specified', it('should use the only source file if only a single one is specified', () => {
() => { expect(findFlatIndexEntryPoint(['/src/index.ts'])).toBe('/src/index.ts'); }); expect(findFlatIndexEntryPoint([AbsoluteFsPath.fromUnchecked('/src/index.ts')]))
.toBe('/src/index.ts');
});
it('should use the shortest source file ending with "index.ts" for multiple files', () => { it('should use the shortest source file ending with "index.ts" for multiple files', () => {
expect(findFlatIndexEntryPoint([ expect(findFlatIndexEntryPoint([
'/src/deep/index.ts', '/src/index.ts', '/index.ts' AbsoluteFsPath.fromUnchecked('/src/deep/index.ts'),
AbsoluteFsPath.fromUnchecked('/src/index.ts'), AbsoluteFsPath.fromUnchecked('/index.ts')
])).toBe('/index.ts'); ])).toBe('/index.ts');
}); });
it('should normalize the path separators for the found entry point',
() => { expect(findFlatIndexEntryPoint(['\\src\\index.ts'])).toBe('/src/index.ts'); });
}); });
}); });

View File

@ -65,6 +65,7 @@ export class NgtscProgram implements api.Program {
this.closureCompilerEnabled = !!options.annotateForClosureCompiler; this.closureCompilerEnabled = !!options.annotateForClosureCompiler;
this.resourceManager = new HostResourceLoader(host, options); this.resourceManager = new HostResourceLoader(host, options);
const shouldGenerateShims = options.allowEmptyCodegenFiles || false; const shouldGenerateShims = options.allowEmptyCodegenFiles || false;
const normalizedRootNames = rootNames.map(n => AbsoluteFsPath.from(n));
this.host = host; this.host = host;
if (host.fileNameToModuleName !== undefined) { if (host.fileNameToModuleName !== undefined) {
this.fileToModuleHost = host as FileToModuleHost; this.fileToModuleHost = host as FileToModuleHost;
@ -74,10 +75,10 @@ export class NgtscProgram implements api.Program {
const generators: ShimGenerator[] = []; const generators: ShimGenerator[] = [];
if (shouldGenerateShims) { if (shouldGenerateShims) {
// Summary generation. // Summary generation.
const summaryGenerator = SummaryGenerator.forRootFiles(rootNames); const summaryGenerator = SummaryGenerator.forRootFiles(normalizedRootNames);
// Factory generation. // Factory generation.
const factoryGenerator = FactoryGenerator.forRootFiles(rootNames); const factoryGenerator = FactoryGenerator.forRootFiles(normalizedRootNames);
const factoryFileMap = factoryGenerator.factoryFileMap; const factoryFileMap = factoryGenerator.factoryFileMap;
this.factoryToSourceInfo = new Map<string, FactoryInfo>(); this.factoryToSourceInfo = new Map<string, FactoryInfo>();
this.sourceToFactorySymbols = new Map<string, Set<string>>(); this.sourceToFactorySymbols = new Map<string, Set<string>>();
@ -94,7 +95,7 @@ export class NgtscProgram implements api.Program {
let entryPoint: string|null = null; let entryPoint: string|null = null;
if (options.flatModuleOutFile !== undefined) { if (options.flatModuleOutFile !== undefined) {
entryPoint = findFlatIndexEntryPoint(rootNames); entryPoint = findFlatIndexEntryPoint(normalizedRootNames);
if (entryPoint === null) { if (entryPoint === null) {
// This error message talks specifically about having a single .ts file in "files". However // This error message talks specifically about having a single .ts file in "files". However
// the actual logic is a bit more permissive. If a single file exists, that will be taken, // the actual logic is a bit more permissive. If a single file exists, that will be taken,

View File

@ -10,6 +10,7 @@ ts_library(
deps = [ deps = [
"//packages/compiler", "//packages/compiler",
"//packages/compiler-cli/src/ngtsc/imports", "//packages/compiler-cli/src/ngtsc/imports",
"//packages/compiler-cli/src/ngtsc/path",
"//packages/compiler-cli/src/ngtsc/util", "//packages/compiler-cli/src/ngtsc/util",
"@npm//@types/node", "@npm//@types/node",
"@npm//typescript", "@npm//typescript",

View File

@ -10,7 +10,7 @@ import * as path from 'path';
import * as ts from 'typescript'; import * as ts from 'typescript';
import {ImportRewriter} from '../../imports'; import {ImportRewriter} from '../../imports';
import {normalizeSeparators} from '../../util/src/path'; import {AbsoluteFsPath} from '../../path/src/types';
import {isNonDeclarationTsPath} from '../../util/src/typescript'; import {isNonDeclarationTsPath} from '../../util/src/typescript';
import {ShimGenerator} from './host'; import {ShimGenerator} from './host';
@ -28,10 +28,10 @@ export class FactoryGenerator implements ShimGenerator {
get factoryFileMap(): Map<string, string> { return this.map; } get factoryFileMap(): Map<string, string> { return this.map; }
recognize(fileName: string): boolean { return this.map.has(fileName); } recognize(fileName: AbsoluteFsPath): boolean { return this.map.has(fileName); }
generate(genFilePath: string, readFile: (fileName: string) => ts.SourceFile | null): ts.SourceFile generate(genFilePath: AbsoluteFsPath, readFile: (fileName: string) => ts.SourceFile | null):
|null { ts.SourceFile|null {
const originalPath = this.map.get(genFilePath) !; const originalPath = this.map.get(genFilePath) !;
const original = readFile(originalPath); const original = readFile(originalPath);
if (original === null) { if (original === null) {
@ -98,11 +98,13 @@ export class FactoryGenerator implements ShimGenerator {
return genFile; return genFile;
} }
static forRootFiles(files: ReadonlyArray<string>): FactoryGenerator { static forRootFiles(files: ReadonlyArray<AbsoluteFsPath>): FactoryGenerator {
const map = new Map<string, string>(); const map = new Map<AbsoluteFsPath, string>();
files.filter(sourceFile => isNonDeclarationTsPath(sourceFile)) files.filter(sourceFile => isNonDeclarationTsPath(sourceFile))
.map(sourceFile => normalizeSeparators(sourceFile)) .forEach(
.forEach(sourceFile => map.set(sourceFile.replace(/\.ts$/, '.ngfactory.ts'), sourceFile)); sourceFile => map.set(
AbsoluteFsPath.fromUnchecked(sourceFile.replace(/\.ts$/, '.ngfactory.ts')),
sourceFile));
return new FactoryGenerator(map); return new FactoryGenerator(map);
} }
} }

View File

@ -7,12 +7,13 @@
*/ */
import * as ts from 'typescript'; import * as ts from 'typescript';
import {AbsoluteFsPath} from '../../path/src/types';
export interface ShimGenerator { export interface ShimGenerator {
/** /**
* Returns `true` if this generator is intended to handle the given file. * Returns `true` if this generator is intended to handle the given file.
*/ */
recognize(fileName: string): boolean; recognize(fileName: AbsoluteFsPath): boolean;
/** /**
* Generate a shim's `ts.SourceFile` for the given original file. * Generate a shim's `ts.SourceFile` for the given original file.
@ -22,8 +23,8 @@ export interface ShimGenerator {
* *
* If `generate` returns null, then the shim generator declines to generate the file after all. * If `generate` returns null, then the shim generator declines to generate the file after all.
*/ */
generate(genFileName: string, readFile: (fileName: string) => ts.SourceFile | null): ts.SourceFile generate(genFileName: AbsoluteFsPath, readFile: (fileName: string) => ts.SourceFile | null):
|null; ts.SourceFile|null;
} }
/** /**
@ -60,14 +61,16 @@ export class GeneratedShimsHostWrapper implements ts.CompilerHost {
shouldCreateNewSourceFile?: boolean|undefined): ts.SourceFile|undefined { shouldCreateNewSourceFile?: boolean|undefined): ts.SourceFile|undefined {
for (let i = 0; i < this.shimGenerators.length; i++) { for (let i = 0; i < this.shimGenerators.length; i++) {
const generator = this.shimGenerators[i]; const generator = this.shimGenerators[i];
if (generator.recognize(fileName)) { // TypeScript internal paths are guaranteed to be POSIX-like absolute file paths.
const absoluteFsPath = AbsoluteFsPath.fromUnchecked(fileName);
if (generator.recognize(absoluteFsPath)) {
const readFile = (originalFile: string) => { const readFile = (originalFile: string) => {
return this.delegate.getSourceFile( return this.delegate.getSourceFile(
originalFile, languageVersion, onError, shouldCreateNewSourceFile) || originalFile, languageVersion, onError, shouldCreateNewSourceFile) ||
null; null;
}; };
return generator.generate(fileName, readFile) || undefined; return generator.generate(absoluteFsPath, readFile) || undefined;
} }
} }
return this.delegate.getSourceFile( return this.delegate.getSourceFile(
@ -98,10 +101,13 @@ export class GeneratedShimsHostWrapper implements ts.CompilerHost {
getNewLine(): string { return this.delegate.getNewLine(); } getNewLine(): string { return this.delegate.getNewLine(); }
fileExists(fileName: string): boolean { fileExists(fileName: string): boolean {
// Consider the file as existing whenever 1) it really does exist in the delegate host, or // Consider the file as existing whenever
// 2) at least one of the shim generators recognizes it. // 1) it really does exist in the delegate host, or
// 2) at least one of the shim generators recognizes it
// Note that we can pass the file name as branded absolute fs path because TypeScript
// internally only passes POSIX-like paths.
return this.delegate.fileExists(fileName) || return this.delegate.fileExists(fileName) ||
this.shimGenerators.some(gen => gen.recognize(fileName)); this.shimGenerators.some(gen => gen.recognize(AbsoluteFsPath.fromUnchecked(fileName)));
} }
readFile(fileName: string): string|undefined { return this.delegate.readFile(fileName); } readFile(fileName: string): string|undefined { return this.delegate.readFile(fileName); }

View File

@ -8,21 +8,21 @@
import * as ts from 'typescript'; import * as ts from 'typescript';
import {normalizeSeparators} from '../../util/src/path'; import {AbsoluteFsPath} from '../../path/src/types';
import {isNonDeclarationTsPath} from '../../util/src/typescript'; import {isNonDeclarationTsPath} from '../../util/src/typescript';
import {ShimGenerator} from './host'; import {ShimGenerator} from './host';
import {generatedModuleName} from './util'; import {generatedModuleName} from './util';
export class SummaryGenerator implements ShimGenerator { export class SummaryGenerator implements ShimGenerator {
private constructor(private map: Map<string, string>) {} private constructor(private map: Map<AbsoluteFsPath, string>) {}
getSummaryFileNames(): string[] { return Array.from(this.map.keys()); } getSummaryFileNames(): string[] { return Array.from(this.map.keys()); }
recognize(fileName: string): boolean { return this.map.has(fileName); } recognize(fileName: AbsoluteFsPath): boolean { return this.map.has(fileName); }
generate(genFilePath: string, readFile: (fileName: string) => ts.SourceFile | null): ts.SourceFile generate(genFilePath: AbsoluteFsPath, readFile: (fileName: string) => ts.SourceFile | null):
|null { ts.SourceFile|null {
const originalPath = this.map.get(genFilePath) !; const originalPath = this.map.get(genFilePath) !;
const original = readFile(originalPath); const original = readFile(originalPath);
if (original === null) { if (original === null) {
@ -77,11 +77,13 @@ export class SummaryGenerator implements ShimGenerator {
return genFile; return genFile;
} }
static forRootFiles(files: ReadonlyArray<string>): SummaryGenerator { static forRootFiles(files: ReadonlyArray<AbsoluteFsPath>): SummaryGenerator {
const map = new Map<string, string>(); const map = new Map<AbsoluteFsPath, string>();
files.filter(sourceFile => isNonDeclarationTsPath(sourceFile)) files.filter(sourceFile => isNonDeclarationTsPath(sourceFile))
.map(sourceFile => normalizeSeparators(sourceFile)) .forEach(
.forEach(sourceFile => map.set(sourceFile.replace(/\.ts$/, '.ngsummary.ts'), sourceFile)); sourceFile => map.set(
AbsoluteFsPath.fromUnchecked(sourceFile.replace(/\.ts$/, '.ngsummary.ts')),
sourceFile));
return new SummaryGenerator(map); return new SummaryGenerator(map);
} }
} }

View File

@ -2669,6 +2669,21 @@ describe('ngtsc behavioral tests', () => {
expect(jsContents).toContain('export * from \'./test\';'); expect(jsContents).toContain('export * from \'./test\';');
}); });
it('should determine the flat module entry-point within multiple root files', () => {
env.tsconfig({
'flatModuleOutFile': 'flat.js',
});
env.write('ignored.ts', 'export const TEST = "this is ignored";');
env.write('index.ts', 'export const ENTRY = "this is the entry";');
env.driveMain();
const jsContents = env.getContents('flat.js');
expect(jsContents)
.toContain(
'export * from \'./index\';',
'Should detect the "index.ts" file as flat module entry-point.');
});
it('should generate a flat module with an id', () => { it('should generate a flat module with an id', () => {
env.tsconfig({ env.tsconfig({
'flatModuleOutFile': 'flat.js', 'flatModuleOutFile': 'flat.js',