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

feat(jsii): Re-implemented jsii to support --watch and produce better error reporting #188

Merged
merged 6 commits into from
Aug 28, 2018

Conversation

RomainMuller
Copy link
Contributor

Re-wrote JSII using the TypeScript compiler services API, integrating with the ts.ProgramBuilder classes so that incremental build is possible. Additonally, error messages leverage TypeScript's built-in diagnostic formatter, and enable the rendering of code excerpts with error messages.

The error message generation can be further improved by maintaining a map of entity (type, method, property, parameter, ...) to node, which could then be used by the Validator class to emit error messages that are tied to the source code. This was omitted for now as the change is already substantial enough.

I have successfully built the AWS CDK using this enhanced compiler. Also, it produces almost no changes in the regression tests' output (only change is that it is now able to carry enum member documentation).

Re-wrote JSII using the TypeScript compiler services API, integrating
with the `ts.ProgramBuilder` classes so that incremental build is
possible. Additonally, error messages leverage `TypeScript`'s built-in
diagnostic formatter, and enable the rendering of code excerpts with
error messages.
@eladb
Copy link
Contributor

eladb commented Aug 28, 2018

  • Rebase
  • Fix title of the PR

@RomainMuller RomainMuller changed the title feat(jsii): Implement --watch support feat(jsii): Re-implemented jsii to support --watch and produce better error reporting Aug 28, 2018
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

Generally this looks great and I wouldn't want it to get stale, so let's merge away! I haven't performed a full deep dive, but since I can see that you haven't needed to change any of the (positive) tests, and I know we have pretty good coverage, I think we can merge this change, and I will provide feedback/propose changes as we work with this new code base.

Great job!

export interface CompilerOptions {
/** The information about the project to be built */
projectInfo: ProjectInfo;
/** Whether the compiler should watch for changes or juts compile once */
Copy link
Contributor

Choose a reason for hiding this comment

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

juts => just

// JavaDoc is very particular about it being @return.
case 'returns': return 'return';
default: return tagName;
public async emit(...files: string[]): Promise<EmitResult | never> {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the use case for files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's used when running the tests against the negatives (the standard behaviour otherwise globs all the negatives, which isn't what we want). There was an other way around (basically, copy each file to a temporary work-dir with a package.json file to run the tests against), but it was a lot more work.

Adding a comment on @param to reflect that.

if (jsii.types) {
for (const fqn of Object.keys(jsii.types)) {
lookup.set(fqn, jsii.types[fqn]);
const SOURCE_DIRS = new Set(['bin', 'lib', 'test']);
Copy link
Contributor

Choose a reason for hiding this comment

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

this should probably be configurable (at some point). issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

*/
public async emit(): Promise<EmitResult> {
this._diagnostics = [];
if (!this.projectInfo.description) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it make more sense to perform these validations in ProjectInfo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to have ProjectInfo remain rather un-opinionated, especially w/r/t optionals. Could make it also emit diagnostics, I suppose. I'm going to hold off on this one for now though.

const mainFile = path.resolve(this.projectInfo.projectRoot, this.projectInfo.types.replace(/\.d\.ts(x?)$/, '.ts$1'));
for (const sourceFile of this.program.getSourceFiles().filter(f => !f.isDeclarationFile)) {
if (sourceFile.fileName !== mainFile) { continue; }
if (LOG.isTraceEnabled()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need to check if trace is enabled? Isn't that just a log level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a log level. The check is to avoid taking on possibly expensive side-effects (this string interpolates with color & all).

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed sounds very expensive. Sometimes those loggers allow you to pass in a function which is lazy evaluated only if the level is enabled...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah but that actually makes the syntax of logging heavier. That's a bit annoying.

Copy link
Contributor

Choose a reason for hiding this comment

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

wrapping every trace call with an "if" statement is also heavy...

};

const validator = new Validator(this.projectInfo, assembly);
const validationResult = await validator.emit();
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the value in making the validator an emitter? What does it emit really? What's the polymorphic benefit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The validator actually further annotates the type specification (for example, it tags theoverridden members).

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yah I think ultimately I want to merge those into the Assembler class entirely. Separating made it easier for me to wrap my head around the two classes of concerns I had to deal with here:

  1. Understanding the type model that I obtain from the tsc API
  2. Make sure we generate valid type definitions for JSII standards

But we can "fail faster" if we merge...


const validator = new Validator(this.projectInfo, assembly);
const validationResult = await validator.emit();
if (!validationResult.emitSkipped) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I usually prefer the special-case not to be the success path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, but it actually forces me to write more code (that is possibly less readable).

}

// Clearing ``this._types`` to allow contents to be garbage-collected.
delete this._types;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe put in finally to avoid "remembering" to do this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah - Actually why wasn't it there already? o_O

for (const prop of moduleType.getProperties()) {
allTypes.push(...await this._visitNode(prop.valueDeclaration, namePrefix.concat(node.name.getText())));
}
if (LOG.isTraceEnabled()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need to check if trace is enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, its a way to make compilation faster by not building large interpolated strings if we're not going to use them.

@RomainMuller RomainMuller merged commit 76472be into master Aug 28, 2018
@RomainMuller RomainMuller deleted the rmuller/tsc-watch-api branch August 28, 2018 15:33
mpiroc added a commit that referenced this pull request Sep 6, 2018
[0.7.2](v0.7.1...v0.7.2) (2018-09-06)

Bug Fixes

* Missing types in JSII assembly, invalid Java code, confusing docs ([#208](#208)) ([b37101f](b37101f)), closes [#175](#175)

Features

* **jsii:** Re-implemented jsii to support --watch and produce better error reporting ([#188](#188)) ([76472be](76472be))
mpiroc added a commit that referenced this pull request Sep 6, 2018
[0.7.2](v0.7.1...v0.7.2) (2018-09-06)

Bug Fixes

* Missing types in JSII assembly, invalid Java code, confusing docs ([#208](#208)) ([b37101f](b37101f)), closes [#175](#175)

Features

* **jsii:** Re-implemented jsii to support --watch and produce better error reporting ([#188](#188)) ([76472be](76472be))
@mpiroc mpiroc mentioned this pull request Sep 6, 2018
mpiroc added a commit that referenced this pull request Sep 6, 2018
* v0.7.2

[0.7.2](v0.7.1...v0.7.2) (2018-09-06)

Bug Fixes

* Missing types in JSII assembly, invalid Java code, confusing docs ([#208](#208)) ([b37101f](b37101f)), closes [#175](#175)

Features

* **jsii:** Re-implemented jsii to support --watch and produce better error reporting ([#188](#188)) ([76472be](76472be))

* Check in changes to tarball expectations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants