Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(compiler): compile .ngfactory.ts files even if nobody reference… #16899

Merged
merged 1 commit into from
May 25, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion build.sh
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ compilePackage() {
$NGC -p ${1}/tsconfig-build.json
echo "====== Create ${1}/../${package_name}.d.ts re-export file for Closure"
echo "$(cat ${LICENSE_BANNER}) ${N} export * from './${package_name}/index'" > ${2}/../${package_name}.d.ts
echo "{\"__symbolic\":\"module\",\"version\":3,\"metadata\":{},\"exports\":[{\"from\":\"./${package_name}/index\"}]}" > ${2}/../${package_name}.metadata.json
echo "{\"__symbolic\":\"module\",\"version\":3,\"metadata\":{},\"exports\":[{\"from\":\"./${package_name}/index\"}],\"bundleRedirect\":true}" > ${2}/../${package_name}.metadata.json
fi

for DIR in ${1}/* ; do
Expand Down
3 changes: 2 additions & 1 deletion integration/hello_world__closure/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@
"noImplicitAny": false,
"sourceMap": false,
"experimentalDecorators": true,
"outDir": "built/src",
"outDir": "built",
"rootDir": ".",
"declaration": true,
"types": []
},
Expand Down
5 changes: 3 additions & 2 deletions packages/compiler-cli/src/codegen.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,17 +36,18 @@ export class CodeGenerator {
public host: ts.CompilerHost, private compiler: compiler.AotCompiler,
private ngCompilerHost: CompilerHost) {}

codegen(): Promise<any> {
codegen(): Promise<string[]> {
return this.compiler
.compileAll(this.program.getSourceFiles().map(
sf => this.ngCompilerHost.getCanonicalFileName(sf.fileName)))
.then(generatedModules => {
generatedModules.forEach(generatedModule => {
return generatedModules.map(generatedModule => {
const sourceFile = this.program.getSourceFile(generatedModule.srcFileUrl);
const emitPath = this.ngCompilerHost.calculateEmitPath(generatedModule.genFileUrl);
const source = GENERATED_META_FILES.test(emitPath) ? generatedModule.source :
generatedModule.source;
this.host.writeFile(emitPath, source, false, () => {}, [sourceFile]);
return emitPath;
});
});
}
Expand Down
12 changes: 10 additions & 2 deletions packages/compiler-cli/src/compiler_host.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ export class CompilerHost implements AotCompilerHost {
private resolverCache = new Map<string, ModuleMetadata[]>();
private bundleIndexCache = new Map<string, boolean>();
private bundleIndexNames = new Set<string>();
private bundleRedirectNames = new Set<string>();
private moduleFileNames = new Map<string, string|null>();
protected resolveModuleNameHost: CompilerHostContext;

Expand Down Expand Up @@ -280,7 +281,8 @@ export class CompilerHost implements AotCompilerHost {
// Check for a bundle index.
if (this.hasBundleIndex(filePath)) {
const normalFilePath = path.normalize(filePath);
return this.bundleIndexNames.has(normalFilePath);
return this.bundleIndexNames.has(normalFilePath) ||
this.bundleRedirectNames.has(normalFilePath);
}
}
return true;
Expand Down Expand Up @@ -331,7 +333,13 @@ export class CompilerHost implements AotCompilerHost {
const metadataFile = typings.replace(DTS, '.metadata.json');
if (this.context.fileExists(metadataFile)) {
const metadata = JSON.parse(this.context.readFile(metadataFile));
if (metadata.importAs) {
if (metadata.bundleRedirect) {
this.bundleRedirectNames.add(typings);
// Note: don't set result = true,
// as this would mark this folder
// as having a bundleIndex too early without
// filling the bundleIndexNames.
} else if (metadata.importAs) {
this.bundleIndexNames.add(typings);
result = true;
}
Expand Down
2 changes: 1 addition & 1 deletion packages/compiler-cli/src/extract_i18n.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import {Extractor} from './extractor';

function extract(
ngOptions: tsc.AngularCompilerOptions, cliOptions: tsc.I18nExtractionCliOptions,
program: ts.Program, host: ts.CompilerHost): Promise<void> {
program: ts.Program, host: ts.CompilerHost) {
return Extractor.create(ngOptions, program, host, cliOptions.locale)
.extract(cliOptions.i18nFormat !, cliOptions.outFile);
}
Expand Down
3 changes: 2 additions & 1 deletion packages/compiler-cli/src/extractor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export class Extractor {
public host: ts.CompilerHost, private ngCompilerHost: CompilerHost,
private program: ts.Program) {}

extract(formatName: string, outFile: string|null): Promise<void> {
extract(formatName: string, outFile: string|null): Promise<string[]> {
// Checks the format and returns the extension
const ext = this.getExtension(formatName);

Expand All @@ -38,6 +38,7 @@ export class Extractor {
const dstFile = outFile || `messages.${ext}`;
const dstPath = path.join(this.options.genDir, dstFile);
this.host.writeFile(dstPath, content, false);
return [dstPath];
});
}

Expand Down
4 changes: 2 additions & 2 deletions packages/compiler-cli/src/ngtools_api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ export class NgTools_InternalApi_NG_2 {
* @internal
* @private
*/
static codeGen(options: NgTools_InternalApi_NG2_CodeGen_Options): Promise<void> {
static codeGen(options: NgTools_InternalApi_NG2_CodeGen_Options): Promise<any> {
const hostContext: CompilerHostContext =
new CustomLoaderModuleResolutionHostAdapter(options.readResource, options.host);
const cliOptions: NgcCliOptions = {
Expand Down Expand Up @@ -141,7 +141,7 @@ export class NgTools_InternalApi_NG_2 {
* @internal
* @private
*/
static extractI18n(options: NgTools_InternalApi_NG2_ExtractI18n_Options): Promise<void> {
static extractI18n(options: NgTools_InternalApi_NG2_ExtractI18n_Options): Promise<any> {
const hostContext: CompilerHostContext =
new CustomLoaderModuleResolutionHostAdapter(options.readResource, options.host);

Expand Down
103 changes: 95 additions & 8 deletions packages/compiler-cli/test/main_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,39 +13,53 @@ import * as path from 'path';

import {main} from '../src/main';

function getNgRootDir() {
const moduleFilename = module.filename.replace(/\\/g, '/');
const distIndex = moduleFilename.indexOf('/dist/all');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe throw if you don't find it? this failure will be hard to track down when we change the output layout next time

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

return moduleFilename.substr(0, distIndex);
}

describe('compiler-cli', () => {
let basePath: string;
let outDir: string;
let write: (fileName: string, content: string) => void;

function writeConfig(tsconfig: string = '{"extends": "./tsconfig-base.json"}') {
write('tsconfig.json', tsconfig);
}

beforeEach(() => {
basePath = makeTempDir();
write = (fileName: string, content: string) => {
fs.writeFileSync(path.join(basePath, fileName), content, {encoding: 'utf-8'});
};
write('tsconfig.json', `{
write('tsconfig-base.json', `{
"compilerOptions": {
"experimentalDecorators": true,
"types": [],
"outDir": "built",
"declaration": true,
"module": "es2015",
"moduleResolution": "node"
},
"angularCompilerOptions": {
"annotateForClosureCompiler": true
},
"files": ["test.ts"]
"moduleResolution": "node",
"lib": ["es6", "dom"]
}
}`);
outDir = path.resolve(basePath, 'built');
const ngRootDir = getNgRootDir();
const nodeModulesPath = path.resolve(basePath, 'node_modules');
fs.mkdirSync(nodeModulesPath);
fs.symlinkSync(path.resolve(__dirname, '..', '..'), path.resolve(nodeModulesPath, '@angular'));
fs.symlinkSync(
path.resolve(ngRootDir, 'dist', 'all', '@angular'),
path.resolve(nodeModulesPath, '@angular'));
fs.symlinkSync(
path.resolve(ngRootDir, 'node_modules', 'rxjs'), path.resolve(nodeModulesPath, 'rxjs'));
});

// Restore reflector since AoT compiler will update it with a new static reflector
afterEach(() => { ɵreflector.updateCapabilities(new ɵReflectionCapabilities()); });

it('should compile without errors', (done) => {
writeConfig();
write('test.ts', 'export const A = 1;');

const mockConsole = {error: (s: string) => {}};
Expand All @@ -62,6 +76,10 @@ describe('compiler-cli', () => {
});

it('should not print the stack trace if user input file does not exist', (done) => {
writeConfig(`{
"extends": "./tsconfig-base.json",
"files": ["test.ts"]
}`);
const mockConsole = {error: (s: string) => {}};

spyOn(mockConsole, 'error');
Expand All @@ -79,6 +97,7 @@ describe('compiler-cli', () => {
});

it('should not print the stack trace if user input file is malformed', (done) => {
writeConfig();
write('test.ts', 'foo;');

const mockConsole = {error: (s: string) => {}};
Expand All @@ -98,6 +117,7 @@ describe('compiler-cli', () => {
});

it('should not print the stack trace if cannot find the imported module', (done) => {
writeConfig();
write('test.ts', `import {MyClass} from './not-exist-deps';`);

const mockConsole = {error: (s: string) => {}};
Expand All @@ -118,6 +138,7 @@ describe('compiler-cli', () => {
});

it('should not print the stack trace if cannot import', (done) => {
writeConfig();
write('empty-deps.ts', 'export const A = 1;');
write('test.ts', `import {MyClass} from './empty-deps';`);

Expand All @@ -139,6 +160,7 @@ describe('compiler-cli', () => {
});

it('should not print the stack trace if type mismatches', (done) => {
writeConfig();
write('empty-deps.ts', 'export const A = "abc";');
write('test.ts', `
import {A} from './empty-deps';
Expand Down Expand Up @@ -179,4 +201,69 @@ describe('compiler-cli', () => {
})
.catch(e => done.fail(e));
});

describe('compile ngfactory files', () => {
it('should report errors for ngfactory files that are not referenced by root files', (done) => {
writeConfig(`{
"extends": "./tsconfig-base.json",
"files": ["mymodule.ts"]
}`);
write('mymodule.ts', `
import {NgModule, Component} from '@angular/core';

@Component({template: '{{unknownProp}}'})
export class MyComp {}

@NgModule({declarations: [MyComp]})
export class MyModule {}
`);

const mockConsole = {error: (s: string) => {}};

const errorSpy = spyOn(mockConsole, 'error');

main({p: basePath}, mockConsole.error)
.then((exitCode) => {
expect(errorSpy).toHaveBeenCalledTimes(1);
expect(errorSpy.calls.mostRecent().args[0])
.toContain('Error at ' + path.join(basePath, 'mymodule.ngfactory.ts'));
expect(errorSpy.calls.mostRecent().args[0])
.toContain(`Property 'unknownProp' does not exist on type 'MyComp'`);

expect(exitCode).toEqual(1);
done();
})
.catch(e => done.fail(e));
});

it('should compile ngfactory files that are not referenced by root files', (done) => {
writeConfig(`{
"extends": "./tsconfig-base.json",
"files": ["mymodule.ts"]
}`);
write('mymodule.ts', `
import {CommonModule} from '@angular/common';
import {NgModule} from '@angular/core';

@NgModule({
imports: [CommonModule]
})
export class MyModule {}
`);

main({p: basePath})
.then((exitCode) => {
expect(exitCode).toEqual(0);

expect(fs.existsSync(path.resolve(outDir, 'mymodule.ngfactory.js'))).toBe(true);
expect(fs.existsSync(path.resolve(
outDir, 'node_modules', '@angular', 'core', 'src',
'application_module.ngfactory.js')))
.toBe(true);

done();
})
.catch(e => done.fail(e));
});
});
});
48 changes: 32 additions & 16 deletions tools/@angular/tsc-wrapped/src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,22 +11,27 @@ import * as path from 'path';
import * as tsickle from 'tsickle';
import * as ts from 'typescript';

import {check, tsc} from './tsc';

import NgOptions from './options';
import {MetadataWriterHost, SyntheticIndexHost} from './compiler_host';
import {CompilerHostAdapter, MetadataBundler} from './bundler';
import {CliOptions} from './cli_options';
import {VinylFile, isVinylFile} from './vinyl_file';
import {MetadataBundler, CompilerHostAdapter} from './bundler';
import {MetadataWriterHost, SyntheticIndexHost} from './compiler_host';
import {privateEntriesToIndex} from './index_writer';
import NgOptions from './options';
import {check, tsc} from './tsc';
import {isVinylFile, VinylFile} from './vinyl_file';

export {UserError} from './tsc';

const DTS = /\.d\.ts$/;
const JS_EXT = /(\.js|)$/;

export type CodegenExtension =
(ngOptions: NgOptions, cliOptions: CliOptions, program: ts.Program, host: ts.CompilerHost) =>
Promise<void>;
const TS_EXT = /\.ts$/;

export interface CodegenExtension {
/**
* Returns the generated file names.
*/
(ngOptions: NgOptions, cliOptions: CliOptions, program: ts.Program,
host: ts.CompilerHost): Promise<string[]>;
}

export function main(
project: string | VinylFile, cliOptions: CliOptions, codegen?: CodegenExtension,
Expand All @@ -46,10 +51,18 @@ export function main(
const basePath = path.resolve(process.cwd(), cliOptions.basePath || projectDir);

// read the configuration options from wherever you store them
const {parsed, ngOptions} = tsc.readConfiguration(project, basePath, options);
let {parsed, ngOptions} = tsc.readConfiguration(project, basePath, options);
ngOptions.basePath = basePath;
const createProgram = (host: ts.CompilerHost, oldProgram?: ts.Program) =>
ts.createProgram(parsed.fileNames, parsed.options, host, oldProgram);
let rootFileNames: string[] = parsed.fileNames.slice(0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we now prefer splat in a new array
[...parsed.fileNames]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

const createProgram = (host: ts.CompilerHost, oldProgram?: ts.Program) => {
return ts.createProgram(rootFileNames.slice(0), parsed.options, host, oldProgram);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

};
const addGeneratedFileName = (genFileName: string) => {
if (genFileName.startsWith(basePath) && TS_EXT.exec(genFileName)) {
rootFileNames.push(genFileName);
}
};

const diagnostics = (parsed.options as any).diagnostics;
if (diagnostics) (ts as any).performance.enable();

Expand Down Expand Up @@ -83,7 +96,7 @@ export function main(
const libraryIndex = `./${path.basename(indexModule)}`;
const content = privateEntriesToIndex(libraryIndex, metadataBundle.privates);
host = new SyntheticIndexHost(host, {name, content, metadata});
parsed.fileNames.push(name);
addGeneratedFileName(name);
}

const tsickleCompilerHostOptions: tsickle.Options = {
Expand All @@ -109,12 +122,14 @@ export function main(
check(errors);

if (ngOptions.skipTemplateCodegen || !codegen) {
codegen = () => Promise.resolve(null);
codegen = () => Promise.resolve([]);
}

if (diagnostics) console.time('NG codegen');
return codegen(ngOptions, cliOptions, program, host).then(() => {
return codegen(ngOptions, cliOptions, program, host).then((genFiles) => {
if (diagnostics) console.timeEnd('NG codegen');
// Add the generated files to the configuration so they will become part of the program.
genFiles.forEach(genFileName => addGeneratedFileName(genFileName));
let definitionsHost: ts.CompilerHost = tsickleCompilerHost;
if (!ngOptions.skipMetadataEmit) {
// if tsickle is not not used for emitting, but we do use the MetadataWriterHost,
Expand All @@ -123,6 +138,7 @@ export function main(
ngOptions.annotationsAs === 'decorators' && !ngOptions.annotateForClosureCompiler;
definitionsHost = new MetadataWriterHost(tsickleCompilerHost, ngOptions, emitJsFiles);
}

// Create a new program since codegen files were created after making the old program
let programWithCodegen = createProgram(definitionsHost, program);
tsc.typeCheck(host, programWithCodegen);
Expand Down
Loading