Skip to content

Commit

Permalink
fix: exit with non-zero exit code if there are compilation errors (#442)
Browse files Browse the repository at this point in the history
In #383 we modified the compiler to continue jsii validation despite
compilation errors but did not propagate the fact that there were
compilation errors.

This resulted in "jsii" exiting with a 0 exit code even in case
of errors.

Fixes #441
  • Loading branch information
Elad Ben-Israel authored Apr 8, 2019
1 parent 23babed commit 6265bf6
Show file tree
Hide file tree
Showing 7 changed files with 14 additions and 26 deletions.
2 changes: 1 addition & 1 deletion packages/jsii/bin/jsii.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ import { VERSION } from '../lib/version';
for (const diagnostic of emitResult.diagnostics) {
utils.logDiagnostic(diagnostic, projectRoot);
}
if (emitResult.emitSkipped) {
if (emitResult.hasErrors) {
process.exit(1);
}
}).catch(e => {
Expand Down
24 changes: 4 additions & 20 deletions packages/jsii/lib/assembler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import path = require('path');
import ts = require('typescript');
import { JSII_DIAGNOSTICS_CODE } from './compiler';
import { getReferencedDocParams, parseSymbolDocumentation } from './docs';
import { Diagnostic, Emitter } from './emitter';
import { Diagnostic, Emitter, EmitResult } from './emitter';
import literate = require('./literate');
import { ProjectInfo } from './project-info';
import utils = require('./utils');
Expand Down Expand Up @@ -90,7 +90,7 @@ export class Assembler implements Emitter {
// Clearing ``this._types`` to allow contents to be garbage-collected.
delete this._types;
try {
return { diagnostics: this._diagnostics, emitSkipped: true };
return { diagnostics: this._diagnostics, hasErrors: true };
} finally {
// Clearing ``this._diagnostics`` to allow contents to be garbage-collected.
delete this._diagnostics;
Expand Down Expand Up @@ -121,7 +121,7 @@ export class Assembler implements Emitter {

const validator = new Validator(this.projectInfo, assembly);
const validationResult = await validator.emit();
if (!validationResult.emitSkipped) {
if (!validationResult.hasErrors) {
const assemblyPath = path.join(this.projectInfo.projectRoot, '.jsii');
LOG.trace(`Emitting assembly: ${colors.blue(assemblyPath)}`);
await fs.writeJson(assemblyPath, _fingerprint(assembly), { replacer: utils.filterEmpty, spaces: 2 });
Expand All @@ -130,7 +130,7 @@ export class Assembler implements Emitter {
try {
return {
diagnostics: [...this._diagnostics, ...validationResult.diagnostics],
emitSkipped: validationResult.emitSkipped
hasErrors: validationResult.hasErrors
};
} finally {
// Clearing ``this._types`` to allow contents to be garbage-collected.
Expand Down Expand Up @@ -1232,22 +1232,6 @@ export class Assembler implements Emitter {
}
}

/**
* The result of ``Assembler#emit()``.
*/
export interface EmitResult {
/**
* @return all diagnostic information produced by the assembler's emit process
*/
readonly diagnostics: ts.Diagnostic[];

/**
* @return true if the assembly was not written to disk (as the consequence
* of errors, which are visible in ``#diagnostics``)
*/
readonly emitSkipped: boolean;
}

function _fingerprint(assembly: spec.Assembly): spec.Assembly {
delete assembly.fingerprint;
assembly = sortJson(assembly);
Expand Down
4 changes: 2 additions & 2 deletions packages/jsii/lib/compiler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -149,12 +149,12 @@ export class Compiler implements Emitter {
// jsii warnings will appear.
const assembler = new Assembler(this.options.projectInfo, program, stdlib);
const assmEmit = await assembler.emit();
if (assmEmit.emitSkipped) {
if (assmEmit.hasErrors) {
LOG.error('Type model errors prevented the JSII assembly from being created');
}

return {
emitSkipped: assmEmit.emitSkipped,
hasErrors: emit.emitSkipped || assmEmit.hasErrors,
diagnostics: [...emit.diagnostics, ...assmEmit.diagnostics]
};
}
Expand Down
2 changes: 1 addition & 1 deletion packages/jsii/lib/emitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export interface Emitter {
*/
export interface EmitResult {
/** Whether the emit was skipped as a result of errors (found in ``diagnostics``) */
emitSkipped: boolean;
hasErrors: boolean;

/** Diagnostic information created when trying to emit stuff */
diagnostics: ReadonlyArray<ts.Diagnostic | Diagnostic>;
Expand Down
2 changes: 1 addition & 1 deletion packages/jsii/lib/validator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export class Validator implements Emitter {
try {
return {
diagnostics: this._diagnostics,
emitSkipped: this._diagnostics.find(diag => diag.category === ts.DiagnosticCategory.Error) != null
hasErrors: this._diagnostics.find(diag => diag.category === ts.DiagnosticCategory.Error) != null
};
} finally {
// Clearing ``this._diagnostics`` to allow contents to be garbage-collected.
Expand Down
4 changes: 4 additions & 0 deletions packages/jsii/test/negatives/neg.compilation-error.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
///!MATCH_ERROR: Cannot find name 'boom'.
///!MATCH_ERROR: Cannot find name 'CompilerErrorIsHere'.

boom! >CompilerErrorIsHere
2 changes: 1 addition & 1 deletion packages/jsii/test/test.negatives.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ for (const source of fs.readdirSync(SOURCE_DIR)) {
test.ok(expectations.length > 0, `Expected error messages should be specified using ${MATCH_ERROR_MARKER}`);
const compiler = new Compiler({ projectInfo: _makeProjectInfo(source), watch: false });
const emitResult = await compiler.emit(path.join(SOURCE_DIR, source));
test.equal(emitResult.emitSkipped, true, `emitSkipped should be true`);
test.equal(emitResult.hasErrors, true, `hasErrors should be true`);
const errors = emitResult.diagnostics.filter(diag => diag.category === ts.DiagnosticCategory.Error);
for (const expectation of expectations) {
test.notEqual(errors.find(e => _messageText(e).indexOf(expectation) !== -1),
Expand Down

0 comments on commit 6265bf6

Please sign in to comment.