fix: Missing types in JSII assembly, invalid Java code, confusing docs #208
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Not doing so could cause type checks to fail randomly (assuming the referred-to type has not been processed just yet by the assembler).
Looking at their properties will only list entities that have an existence in the Javascript world, which is not the case of interfaces, for example.
The anonymous class generated by the builders included a verbatim field name from the type specification, which could include Java keywords (such as assert). Appended a $ in front to avoid all collisions there, while also avoiding to collide with the parent builder's _fields.
The `ref` of array types would not include the [], causing them to render in confusing ways in the documentation. Additionally, arrays of union types would only have the [] on the last of the options, which was incorrect. Added parentheses around the possible element types in order to clarify the situation.
RomainMuller
changed the title
eladb
reviewed
Sep 3, 2018
| ts.DiagnosticCategory.Error, | ||
| `Unable to resolve type ${jsiiType.base!.fqn} (base type of ${jsiiType.fqn})`); | ||
| } else if (spec.isClassType(baseType)) { | ||
| jsiiType.initializer = baseType.initializer; |
There was a problem hiding this comment.
I am concerned about defer. For example, the fact that you only assign the initializer here looks like a potential race condition. How do I know what order these defer statements actually execution? Now initializer is a moving target!
I am okay with using defer for validation (maybe should be renamed to validate), but it seems very fragile for mutations of the model
added 3 commits
September 5, 2018 09:35
And fix a bug which failed complication if a namespace contained /only/ an interface.
eladb
reviewed
Sep 5, 2018
added 3 commits
September 5, 2018 12:07
eladb
approved these changes
Sep 5, 2018
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))
Merged
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
jsiiwould omit interfaces defined within namespaces because of the way namespaces were processed (using members instead of listing all exports).jsiitype validations could, in some rare cases, happen too early, attempting to dereference types that hadn't been processed yet.jsii-pacmakgenerator for Java could generate property names that were reserved words (e.g:assert).jsii-pacmakgenerator for Sphinx would generate confusing (or incorrect) type documentation for entities of array types, and particularly so for arrays of unions.Should probably add (I don't believe I'll be able to do this before I go on holidays 😅):
jsii-calcor its dependencies that exercise the interface-in-namespace.jsii-calcor its dependencies that exercise reserved words of various languages.