feat(jsii): Re-implemented jsii to support --watch and produce better error reporting by RomainMuller · Pull Request #188 · aws/jsii

RomainMuller · 2018-08-17T14:15:47Z

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 · 2018-08-28T07:57:55Z

Rebase
Fix title of the PR

eladb

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!

eladb · 2018-08-28T09:21:51Z

packages/jsii/lib/compiler.ts

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


juts => just

eladb · 2018-08-28T09:22:35Z

packages/jsii/lib/compiler.ts

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


What's the use case for files?

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.

eladb · 2018-08-28T09:25:36Z

packages/jsii/lib/compiler.ts

-        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']);


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

eladb · 2018-08-28T09:27:09Z

packages/jsii/lib/assembler.ts

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


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

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.

eladb · 2018-08-28T09:28:04Z

packages/jsii/lib/assembler.ts

+        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()) {


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

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

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

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

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

eladb · 2018-08-28T09:29:18Z

packages/jsii/lib/assembler.ts

+        };
+
+        const validator = new Validator(this.projectInfo, assembly);
+        const validationResult = await validator.emit();


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

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

That sounds wrong.

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:

Understanding the type model that I obtain from the tsc API

Make sure we generate valid type definitions for JSII standards

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

eladb · 2018-08-28T09:29:54Z

packages/jsii/lib/assembler.ts

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


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

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

eladb · 2018-08-28T09:30:12Z

packages/jsii/lib/assembler.ts

+        }
+
+        // Clearing ``this._types`` to allow contents to be garbage-collected.
+        delete this._types;


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

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

eladb · 2018-08-28T09:31:40Z

packages/jsii/lib/assembler.ts

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


Why do you need to check if trace is enabled?

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

[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))

* 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.

RomainMuller requested review from eladb and rix0rrr August 17, 2018 14:15

RomainMuller mentioned this pull request Aug 22, 2018

Forces me to use a license field #195

Closed

RomainMuller force-pushed the rmuller/tsc-watch-api branch from ff6f7ff to 7dca3ba Compare August 25, 2018 23:39

RomainMuller mentioned this pull request Aug 25, 2018

JSII seems to not prevent a read-write property from being downgraded to read-only #199

Closed

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

eladb approved these changes Aug 28, 2018

View reviewed changes

RomainMuller added 5 commits August 28, 2018 13:38

Merge remote-tracking branch 'origin/master' into rmuller/tsc-watch-api

f6ffef1

PR Feedback

bc116c1

Merge remote-tracking branch 'origin/master' into rmuller/tsc-watch-api

0ae6fe8

Revert to lerna 2.11.0

c6371eb

Fix bundledDependencies

60718d4

RomainMuller merged commit 76472be into master Aug 28, 2018

RomainMuller deleted the rmuller/tsc-watch-api branch August 28, 2018 15:33

mpiroc mentioned this pull request Sep 6, 2018

Bump version to v0.7.2 #212

Merged

Conversation

RomainMuller commented Aug 17, 2018

Uh oh!

eladb commented Aug 28, 2018

Uh oh!

eladb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants