Skip to content

Commit

Permalink
fix(jsii): --watch causes immediate failure (#1150)
Browse files Browse the repository at this point in the history 
* fix(jsii): `--watch` causes immediate failure

A change in the TypeScript compiler API around how the compiler host
reacts when invoked in `--watch` mode caused a failure in `jsii` due to
the method changing behaviors from not returning at all when watching to
instead returning an object that can be used to inspect/influence the
background watch process.

Changed the behavior of the `Compiler` interface so it now acts as an
`EventEmitter` when operating in `--watch` mode, and introduced a test
that confirms that operating with `--watch` results in the correct
behavior of watching for file changes and dynamically re-compiling.

Fixes #1149

* some PR feedback

* fix dummy project

* better encapsulate watch API

* Update packages/jsii/bin/jsii.ts

Co-Authored-By: Elad Ben-Israel <benisrae@amazon.com>

* expose ts API instead of wrapping it

Co-authored-by: Elad Ben-Israel <benisrae@amazon.com>
  • Loading branch information
@RomainMuller @mergify
2 people authored and mergify[bot] committed Dec 30, 2019
1 parent bb38652 commit 6bdf7d7
Show file tree
Hide file tree
Showing 8 changed files with 249 additions and 98 deletions.
17 changes: 10 additions & 7 deletions packages/jsii/bin/jsii.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import * as log4js from 'log4js';
import * as path from 'path';
import * as process from 'process';
import * as yargs from 'yargs';
import { Compiler, DIAGNOSTICS } from '../lib/compiler';
import { Compiler } from '../lib/compiler';
import { loadProjectInfo } from '../lib/project-info';
import * as utils from '../lib/utils';
import { VERSION } from '../lib/version';
Expand Down Expand Up @@ -57,12 +57,14 @@ const warningTypes = Object.keys(enabledWarnings);

const compiler = new Compiler({
projectInfo,
watch: argv.watch,
projectReferences: argv['project-references'],
failOnWarnings: argv['fail-on-warnings'],
});

return { projectRoot, emitResult: await compiler.emit() };
const result = argv.watch
? compiler.watch()
: compiler.emit();
return { projectRoot, emitResult: await result };
})().then(({ projectRoot, emitResult }) => {
for (const diagnostic of emitResult.diagnostics) {
utils.logDiagnostic(diagnostic, projectRoot);
Expand All @@ -82,19 +84,20 @@ function _configureLog4js(verbosity: number) {
type: 'stderr',
layout: { type: 'colored' }
},
diagnostics: {
[utils.DIAGNOSTICS]: {
type: 'stdout',
layout: { type: 'messagePassThrough' }
}
},
categories: {
default: { appenders: ['console'], level: _logLevel() },
[DIAGNOSTICS]: { appenders: ['diagnostics'], level: _logLevel() }
// The diagnostics logger must be set to INFO or more verbose, or watch won't show important messages
[utils.DIAGNOSTICS]: { appenders: ['diagnostics'], level: _logLevel(Math.max(verbosity, 1)) }
}
});

function _logLevel(): keyof log4js.Levels {
switch (verbosity) {
function _logLevel(verbosityLevel = verbosity): keyof log4js.Levels {
switch (verbosityLevel) {
case 0: return 'WARN';
case 1: return 'INFO';
case 2: return 'DEBUG';
Expand Down
4 changes: 2 additions & 2 deletions packages/jsii/lib/assembler.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@ import * as spec from '@jsii/spec';
import * as log4js from 'log4js';
import * as path from 'path';
import * as ts from 'typescript';
import { JSII_DIAGNOSTICS_CODE } from './compiler';
import { getReferencedDocParams, parseSymbolDocumentation } from './docs';
import { Diagnostic, EmitResult, Emitter } from './emitter';
import * as literate from './literate';
import { ProjectInfo } from './project-info';
import { isReservedName } from './reserved-words';
import { JSII_DIAGNOSTICS_CODE } from './utils';
import { Validator } from './validator';
import { SHORT_VERSION, VERSION } from './version';
import { enabledWarnings } from './warnings';
Expand Down Expand Up @@ -343,7 +343,7 @@ export class Assembler implements Emitter {
if (LOG.isTraceEnabled()) { LOG.trace(`Leaving namespace: ${colors.cyan([...context.namespace, name].join('.'))}`); }
return allTypes;
} else {
this._diagnostic(node, ts.DiagnosticCategory.Message, `Skipping ${ts.SyntaxKind[node.kind]} node`);
this._diagnostic(node, ts.DiagnosticCategory.Message, `Ignoring ${ts.SyntaxKind[node.kind]} node (it cannot be represented int he jsii type model)`);
}

if (!jsiiType) { return []; }
Expand Down
207 changes: 128 additions & 79 deletions packages/jsii/lib/compiler.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,51 +72,30 @@ export class Compiler implements Emitter {
}

/**
* Compiles the configured program.
*
* @param files can be specified to override the standard source code location logic. Useful for example when testing "negatives".
*/
public async emit(...files: string[]): Promise<EmitResult | never> {
await this.buildTypeScriptConfig();
await this.writeTypeScriptConfig();
this.rootFiles = this.determineSources(files);

if (this.options.watch) {
if (files.length > 0) {
throw new Error('Files cannot be specified in watch mode!');
}
return this._startWatch();
}
* Compiles the configured program.
*
* @param files can be specified to override the standard source code location logic. Useful for example when testing "negatives".
*/
public async emit(...files: string[]): Promise<EmitResult> {
await this._prepareForBuild(...files);
return this._buildOnce();

}

/**
* Do a single build
*/
private async _buildOnce(): Promise<EmitResult> {
if (!this.compilerHost.getDefaultLibLocation) {
throw new Error('No default library location was found on the TypeScript compiler host!');
}

const tsconf = this.typescriptConfig!;
const pi = this.options.projectInfo;

const prog = ts.createProgram({
rootNames: this.rootFiles.concat(_pathOfLibraries(this.compilerHost)),
options: { ...pi.tsc, ...COMPILER_OPTIONS },
// Make the references absolute for the compiler
projectReferences: tsconf.references?.map(ref => ({ path: path.resolve(ref.path) })),
host: this.compilerHost
});

return this._consumeProgram(prog, this.compilerHost.getDefaultLibLocation());
}

* Watches for file-system changes and dynamically recompiles the project as needed. In non-blocking mode, this
* returns the TypeScript watch handle for the application to use.
*
* @internal
*/
public async watch(opts: NonBlockingWatchOptions): Promise<ts.Watch<ts.BuilderProgram>>;
/**
* Start a watch on the config that has been written to disk
*/
private async _startWatch(): Promise<never> {
* Watches for file-system changes and dynamically recompiles the project as needed. In blocking mode, this results
* in a never-resolving promise.
*/
public async watch(): Promise<never>;
public async watch(opts?: NonBlockingWatchOptions): Promise<ts.Watch<ts.BuilderProgram> | never> {
await this._prepareForBuild();

const pi = this.options.projectInfo;
const projectRoot = pi.projectRoot;
const host = ts.createWatchCompilerHost(
Expand All @@ -126,7 +105,10 @@ export class Compiler implements Emitter {
...COMPILER_OPTIONS,
noEmitOnError: false,
},
{ ...ts.sys, getCurrentDirectory() { return projectRoot; } }
{ ...ts.sys, getCurrentDirectory() { return projectRoot; } },
ts.createEmitAndSemanticDiagnosticsBuilderProgram,
opts?.reportDiagnostics,
opts?.reportWatchStatus,
);
if (!host.getDefaultLibLocation) {
throw new Error('No default library location was found on the TypeScript compiler host!');
Expand All @@ -140,10 +122,50 @@ export class Compiler implements Emitter {
}

if (orig) { orig.call(host, builderProgram); }
if (opts?.compilationComplete) { await opts.compilationComplete(emitResult); }
};
ts.createWatchProgram(host);
// Previous call never returns
return Promise.reject(new Error('Unexpectedly returned from createWatchProgram'));
const watch = ts.createWatchProgram(host);

if (opts?.nonBlocking) {
// In non-blocking mode, returns the handle to the TypeScript watch interface.
return watch;
}
// In blocking mode, returns a never-resolving promise.
return new Promise<never>(() => null);
}

/**
* Prepares the project for build, by creating the necessary configuration
* file(s), and assigning the relevant root file(s).
*
* @param files the files that were specified as input in the CLI invocation.
*/
private async _prepareForBuild(...files: string[]) {
await this.buildTypeScriptConfig();
await this.writeTypeScriptConfig();
this.rootFiles = this.determineSources(files);
}

/**
* Do a single build
*/
private async _buildOnce(): Promise<EmitResult> {
if (!this.compilerHost.getDefaultLibLocation) {
throw new Error('No default library location was found on the TypeScript compiler host!');
}

const tsconf = this.typescriptConfig!;
const pi = this.options.projectInfo;

const prog = ts.createProgram({
rootNames: this.rootFiles.concat(_pathOfLibraries(this.compilerHost)),
options: { ...pi.tsc, ...COMPILER_OPTIONS },
// Make the references absolute for the compiler
projectReferences: tsconf.references?.map(ref => ({ path: path.resolve(ref.path) })),
host: this.compilerHost
});

return this._consumeProgram(prog, this.compilerHost.getDefaultLibLocation());
}

private async _consumeProgram(program: ts.Program, stdlib: string): Promise<EmitResult> {
Expand Down Expand Up @@ -175,10 +197,10 @@ export class Compiler implements Emitter {
}

/**
* Build the TypeScript config object
*
* This is the object that will be written to disk.
*/
* Build the TypeScript config object
*
* This is the object that will be written to disk.
*/
private async buildTypeScriptConfig() {
let references: string[] | undefined;
let composite: boolean | undefined;
Expand Down Expand Up @@ -212,10 +234,10 @@ export class Compiler implements Emitter {
}

/**
* Creates a `tsconfig.json` file to improve the IDE experience.
*
* @return the fully qualified path to the ``tsconfig.json`` file
*/
* Creates a `tsconfig.json` file to improve the IDE experience.
*
* @return the fully qualified path to the ``tsconfig.json`` file
*/
private async writeTypeScriptConfig(): Promise<void> {
const commentKey = '_generated_by_jsii_';
const commentValue = 'Generated by jsii - safe to delete, and ideally should be in .gitignore';
Expand All @@ -233,15 +255,15 @@ export class Compiler implements Emitter {
}

/**
* Find all dependencies that look like TypeScript projects.
*
* Enumerate all dependencies, if they have a tsconfig.json file with
* "composite: true" we consider them project references.
*
* (Note: TypeScript seems to only correctly find transitive project references
* if there's an "index" tsconfig.json of all projects somewhere up the directory
* tree)
*/
* Find all dependencies that look like TypeScript projects.
*
* Enumerate all dependencies, if they have a tsconfig.json file with
* "composite: true" we consider them project references.
*
* (Note: TypeScript seems to only correctly find transitive project references
* if there's an "index" tsconfig.json of all projects somewhere up the directory
* tree)
*/
private async findProjectReferences(): Promise<string[]> {
const pkg = this.options.projectInfo.packageJson;

Expand Down Expand Up @@ -277,12 +299,12 @@ export class Compiler implements Emitter {
}

/**
* Find source files using the same mechanism that the TypeScript compiler itself uses.
*
* Respects includes/excludes/etc.
*
* This makes it so that running 'tsc' and running 'jsii' has the same behavior.
*/
* Find source files using the same mechanism that the TypeScript compiler itself uses.
*
* Respects includes/excludes/etc.
*
* This makes it so that running 'tsc' and running 'jsii' has the same behavior.
*/
private determineSources(files: string[]): string[] {
const ret = new Array<string>();

Expand All @@ -298,18 +320,18 @@ export class Compiler implements Emitter {
}

/**
* Resolve the given dependency name from the current package, and find the associated tsconfig.json location
*
* Because we have the following potential directory layout:
*
* package/node_modules/some_dependency
* package/tsconfig.json
*
* We resolve symlinks and only find a "TypeScript" dependency if doesn't have 'node_modules' in
* the path after resolving symlinks (i.e., if it's a peer package in the same monorepo).
*
* Returns undefined if no such tsconfig could be found.
*/
* Resolve the given dependency name from the current package, and find the associated tsconfig.json location
*
* Because we have the following potential directory layout:
*
* package/node_modules/some_dependency
* package/tsconfig.json
*
* We resolve symlinks and only find a "TypeScript" dependency if doesn't have 'node_modules' in
* the path after resolving symlinks (i.e., if it's a peer package in the same monorepo).
*
* Returns undefined if no such tsconfig could be found.
*/
private async findMonorepoPeerTsconfig(depName: string): Promise<string | undefined> {
const paths = nodeJsCompatibleSearchPaths(this.options.projectInfo.projectRoot);

Expand All @@ -329,6 +351,33 @@ export class Compiler implements Emitter {
}
}

/**
* Options for Watch in non-blocking mode.
*
* @internal
*/
export interface NonBlockingWatchOptions {
/**
* Signals non-blocking execution
*/
readonly nonBlocking: true;

/**
* Configures the diagnostics reporter
*/
readonly reportDiagnostics: ts.DiagnosticReporter;

/**
* Configures the watch status reporter
*/
readonly reportWatchStatus: ts.WatchStatusReporter;

/**
* This hook gets invoked when a compilation cycle (complete with Assembler execution) completes.
*/
readonly compilationComplete: (emitResult: EmitResult) => void | Promise<void>;
}

function _pathOfLibraries(host: ts.CompilerHost | ts.WatchCompilerHost<any>): string[] {
if (!COMPILER_OPTIONS.lib || COMPILER_OPTIONS.lib.length === 0) { return []; }
const lib = host.getDefaultLibLocation?.();
Expand Down
2 changes: 1 addition & 1 deletion packages/jsii/lib/helpers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export async function sourceToAssemblyHelper(source: string, cb?: (obj: PackageI
return inTempDir(async () => {
const fileName = 'index.ts';
await fs.writeFile(fileName, source, { encoding: 'utf-8' });
const compiler = new Compiler({ projectInfo: await makeProjectInfo(fileName, cb), watch: false });
const compiler = new Compiler({ projectInfo: await makeProjectInfo(fileName, cb) });
const emitResult = await compiler.emit();

const errors = emitResult.diagnostics.filter(d => d.category === DiagnosticCategory.Error);
Expand Down
6 changes: 5 additions & 1 deletion packages/jsii/lib/project-info.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,11 @@ const ASSEMBLY_CACHE = new Map<string, spec.Assembly>();
*/
async function loadAndValidateAssembly(jsiiFileName: string): Promise<spec.Assembly> {
if (!ASSEMBLY_CACHE.has(jsiiFileName)) {
ASSEMBLY_CACHE.set(jsiiFileName, spec.validateAssembly(await fs.readJson(jsiiFileName)));
try {
ASSEMBLY_CACHE.set(jsiiFileName, spec.validateAssembly(await fs.readJson(jsiiFileName)));
} catch (e) {
throw new Error(`Error loading ${jsiiFileName}: ${e}`);
}
}
return ASSEMBLY_CACHE.get(jsiiFileName)!;
}
Expand Down
Loading

0 comments on commit 6bdf7d7

Please sign in to comment.