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(ngcc): handle compilation diagnostics #31996

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,13 @@ export class DecorationAnalyzer {

protected analyzeClass(symbol: ClassSymbol): AnalyzedClass|null {
const decorators = this.reflectionHost.getDecoratorsOfSymbol(symbol);
return analyzeDecorators(symbol, decorators, this.handlers);
const analyzedClass = analyzeDecorators(symbol, decorators, this.handlers);
if (analyzedClass !== null && analyzedClass.diagnostics !== undefined) {
for (const diagnostic of analyzedClass.diagnostics) {
this.diagnosticHandler(diagnostic);
}
}
return analyzedClass;
}

protected migrateFile(migrationHost: MigrationHost, analyzedFile: AnalyzedFile): void {
Expand Down
19 changes: 15 additions & 4 deletions packages/compiler-cli/ngcc/src/analysis/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,12 @@
* found in the LICENSE file at https://angular.io/license
*/
import * as ts from 'typescript';

import {isFatalDiagnosticError} from '../../../src/ngtsc/diagnostics';
import {AbsoluteFsPath, absoluteFromSourceFile, relative} from '../../../src/ngtsc/file_system';
import {ClassSymbol, Decorator} from '../../../src/ngtsc/reflection';
import {DecoratorHandler, DetectResult, HandlerPrecedence} from '../../../src/ngtsc/transform';

import {AnalyzedClass, MatchingHandler} from './types';

export function isWithinPackage(packagePath: AbsoluteFsPath, sourceFile: ts.SourceFile): boolean {
Expand Down Expand Up @@ -59,11 +62,19 @@ export function analyzeDecorators(
const matches: {handler: DecoratorHandler<any, any>, analysis: any}[] = [];
const allDiagnostics: ts.Diagnostic[] = [];
for (const {handler, detected} of detections) {
const {analysis, diagnostics} = handler.analyze(declaration, detected.metadata);
if (diagnostics !== undefined) {
allDiagnostics.push(...diagnostics);
try {
const {analysis, diagnostics} = handler.analyze(declaration, detected.metadata);
if (diagnostics !== undefined) {
allDiagnostics.push(...diagnostics);
}
matches.push({handler, analysis});
} catch (e) {
if (isFatalDiagnosticError(e)) {
allDiagnostics.push(e.toDiagnostic());
} else {
throw e;
}
}
matches.push({handler, analysis});
}
return {
name: symbol.name,
Expand Down
16 changes: 13 additions & 3 deletions packages/compiler-cli/ngcc/src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import * as ts from 'typescript';

import {AbsoluteFsPath, FileSystem, absoluteFrom, dirname, getFileSystem, resolve} from '../../src/ngtsc/file_system';

import {CommonJsDependencyHost} from './dependencies/commonjs_dependency_host';
Expand All @@ -27,7 +29,6 @@ import {FileWriter} from './writing/file_writer';
import {InPlaceFileWriter} from './writing/in_place_file_writer';
import {NewEntryPointFileWriter} from './writing/new_entry_point_file_writer';


/**
* The options to configure the ngcc compiler.
*/
Expand Down Expand Up @@ -171,8 +172,17 @@ export function mainNgcc(

logger.info(`Compiling ${entryPoint.name} : ${formatProperty} as ${format}`);

const transformedFiles = transformer.transform(bundle);
fileWriter.writeBundle(bundle, transformedFiles, formatPropertiesToMarkAsProcessed);
const result = transformer.transform(bundle);
if (result.success) {
if (result.diagnostics.length > 0) {
logger.warn(ts.formatDiagnostics(result.diagnostics, bundle.src.host));
}
fileWriter.writeBundle(bundle, result.transformedFiles, formatPropertiesToMarkAsProcessed);
} else {
const errors = ts.formatDiagnostics(result.diagnostics, bundle.src.host);
throw new Error(
`Failed to compile entry-point ${entryPoint.name} due to compilation errors:\n${errors}`);
}

onTaskCompleted(task, TaskProcessingOutcome.Processed);
};
Expand Down
33 changes: 27 additions & 6 deletions packages/compiler-cli/ngcc/src/packages/transformer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/
import * as ts from 'typescript';

import {FileSystem} from '../../../src/ngtsc/file_system';
import {DecorationAnalyzer} from '../analysis/decoration_analyzer';
import {ModuleWithProvidersAnalyses, ModuleWithProvidersAnalyzer} from '../analysis/module_with_providers_analyzer';
Expand All @@ -27,8 +28,17 @@ import {Renderer} from '../rendering/renderer';
import {RenderingFormatter} from '../rendering/rendering_formatter';
import {UmdRenderingFormatter} from '../rendering/umd_rendering_formatter';
import {FileToWrite} from '../rendering/utils';

import {EntryPointBundle} from './entry_point_bundle';

export type TransformResult = {
success: true; diagnostics: ts.Diagnostic[]; transformedFiles: FileToWrite[];
} |
{
success: false;
diagnostics: ts.Diagnostic[];
};

/**
* A Package is stored in a directory on disk and that directory can contain one or more package
* formats - e.g. fesm2015, UMD, etc. Additionally, each package provides typings (`.d.ts` files).
Expand Down Expand Up @@ -58,12 +68,17 @@ export class Transformer {
* @param bundle the bundle to transform.
* @returns information about the files that were transformed.
*/
transform(bundle: EntryPointBundle): FileToWrite[] {
transform(bundle: EntryPointBundle): TransformResult {
const reflectionHost = this.getHost(bundle);

// Parse and analyze the files.
const {decorationAnalyses, switchMarkerAnalyses, privateDeclarationsAnalyses,
moduleWithProvidersAnalyses} = this.analyzeProgram(reflectionHost, bundle);
moduleWithProvidersAnalyses, diagnostics} = this.analyzeProgram(reflectionHost, bundle);

// Bail if the analysis produced any errors.
if (hasErrors(diagnostics)) {
return {success: false, diagnostics};
}

// Transform the source files and source maps.
const srcFormatter = this.getRenderingFormatter(reflectionHost, bundle);
Expand All @@ -81,7 +96,7 @@ export class Transformer {
renderedFiles = renderedFiles.concat(renderedDtsFiles);
}

return renderedFiles;
return {success: true, diagnostics, transformedFiles: renderedFiles};
}

getHost(bundle: EntryPointBundle): NgccReflectionHost {
Expand Down Expand Up @@ -127,8 +142,10 @@ export class Transformer {
new SwitchMarkerAnalyzer(reflectionHost, bundle.entryPoint.package);
const switchMarkerAnalyses = switchMarkerAnalyzer.analyzeProgram(bundle.src.program);

const decorationAnalyzer =
new DecorationAnalyzer(this.fs, bundle, reflectionHost, referencesRegistry);
const diagnostics: ts.Diagnostic[] = [];
const decorationAnalyzer = new DecorationAnalyzer(
this.fs, bundle, reflectionHost, referencesRegistry,
diagnostic => diagnostics.push(diagnostic));
const decorationAnalyses = decorationAnalyzer.analyzeProgram();

const moduleWithProvidersAnalyzer =
Expand All @@ -142,14 +159,18 @@ export class Transformer {
privateDeclarationsAnalyzer.analyzeProgram(bundle.src.program);

return {decorationAnalyses, switchMarkerAnalyses, privateDeclarationsAnalyses,
moduleWithProvidersAnalyses};
moduleWithProvidersAnalyses, diagnostics};
}
}

export function hasErrors(diagnostics: ts.Diagnostic[]) {
return diagnostics.some(d => d.category === ts.DiagnosticCategory.Error);
}

interface ProgramAnalyses {
decorationAnalyses: Map<ts.SourceFile, CompiledFile>;
switchMarkerAnalyses: SwitchMarkerAnalyses;
privateDeclarationsAnalyses: ExportInfo[];
moduleWithProvidersAnalyses: ModuleWithProvidersAnalyses|null;
diagnostics: ts.Diagnostic[];
}
35 changes: 35 additions & 0 deletions packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,41 @@ runInEachFileSystem(() => {
});
});

describe('diagnostics', () => {
it('should fail with formatted diagnostics when an error diagnostic is produced', () => {
loadTestFiles([
{
name: _('/node_modules/fatal-error/package.json'),
contents: '{"name": "fatal-error", "es2015": "./index.js", "typings": "./index.d.ts"}',
},
{name: _('/node_modules/fatal-error/index.metadata.json'), contents: 'DUMMY DATA'},
{
name: _('/node_modules/fatal-error/index.js'),
contents: `
import {Component} from '@angular/core';
export class FatalError {}
FatalError.decorators = [
{type: Component, args: [{selector: 'fatal-error'}]}
];
`,
},
{
name: _('/node_modules/fatal-error/index.d.ts'),
contents: `
export declare class FatalError {}
`,
},
]);
expect(() => mainNgcc({
basePath: '/node_modules',
targetEntryPointPath: 'fatal-error',
propertiesToConsider: ['es2015']
}))
.toThrowError(
/^Failed to compile entry-point fatal-error due to compilation errors:\nnode_modules\/fatal-error\/index\.js\(5,17\): error TS-992001: component is missing a template\r?\n$/);
});
});

describe('logger', () => {
it('should log info message to the console by default', () => {
const consoleInfoSpy = spyOn(console, 'info');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,15 @@
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
/// <reference types="node" />
import * as os from 'os';
import {NodeJSFileSystem} from '../../src/node_js_file_system';
import {AbsoluteFsPath, PathSegment, PathString} from '../../src/types';

import {MockFileSystem} from './mock_file_system';

const isWindows = os.platform() === 'win32';

export class MockFileSystemNative extends MockFileSystem {
constructor(cwd: AbsoluteFsPath = '/' as AbsoluteFsPath) { super(undefined, cwd); }

Expand Down Expand Up @@ -41,6 +45,15 @@ export class MockFileSystemNative extends MockFileSystem {
}

normalize<T extends PathString>(path: T): T {
// When running in Windows, absolute paths are normalized to always include a drive letter. This
// ensures that rooted posix paths used in tests will be normalized to real Windows paths, i.e.
// including a drive letter. Note that the same normalization is done in emulated Windows mode
// (see `MockFileSystemWindows`) so that the behavior is identical between native Windows and
// emulated Windows mode.
if (isWindows) {
path = path.replace(/^[\/\\]/i, 'C:/') as T;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: The i flag is redundant (as is escaping / inside [...] 😁).

}

return NodeJSFileSystem.prototype.normalize.call(this, path) as T;
}

Expand Down