fix(jsii): use base interfaces for 'datatype' property#265
Conversation
If an interface inherits from a non-datatype interface, it should no longer be classified as a datatype interface itself. Making this change requires that information about the base classes has already been determined, so I introduced an ordering mechanism for 'deferred's. Fixes #264.
packages/jsii/lib/assembler.ts
Outdated
| */ | ||
| private _defer(cb: () => void) { | ||
| this._deferred.push(cb.bind(this)); | ||
| private _defer(fqn: string, depFqns: string[], cb: () => void) { |
There was a problem hiding this comment.
Should we just inline this method into _deferUntilTypeAvailable so people won't be tempted to use this sketchy mechanism
There was a problem hiding this comment.
I don't agree that it's sketchy--it's running some analysis in a guaranteed order. I was debating whether to rewrite the whole assembler into two phases, first collect all types and relationships between types, then do the regular processing in order.
Part of the processing doesn't require the base types though, so I ultimately decided it would be a lower-impact change to leave the standalone analysis in the first pass and only do the analysis that requires base types in the second pass--and the "deferred" mechanism could be extended easily enough to accomodate the ordering required for that second pass.
Now, whether or not it should be inlined is another case. In principle the ordered analysis method is more general than it's being used for right now. It doesn't NEED to work on NamedTypeReferences that are dereferenced... just so happens that all places where it's being used use that pattern, so I extracted that out again. I don't think it hurts to show there are two layers to it.
packages/jsii/lib/assembler.ts
Outdated
|
|
||
| interface DeferredRecord { | ||
| fqn: string; | ||
| depFqns: string[]; |
There was a problem hiding this comment.
Some doc comments would be welcome here... Is "dep" for "depended" or "depending" ?
packages/jsii/lib/assembler.ts
Outdated
| * that case anyway. | ||
| */ | ||
| // tslint:disable-next-line:max-line-length | ||
| private _deferUntilTypeAvailable(fqn: string, baseTypes: NamedTypeReference[], referencingNode: ts.Node, cb: (...xs: spec.Type[]) => void) { |
There was a problem hiding this comment.
Should it not be _deferUntilType+s+Available?
I would also like it if the doc had @params.
* **jsii:** use base interfaces for 'datatype' property ([#265](#265)) ([1c56902](1c56902)), closes [#264](#264) * **jsii:** use default jsx compiler options ([#261](#261)) ([bf1f586](bf1f586)), closes [aws/aws-cdk#830](aws/aws-cdk#830) * match behavioral interface to 'I'-prefix ([#271](#271)) ([03103f3](03103f3)) * require distinct argument and property names ([#272](#272)) ([4d2f268](4d2f268)), closes [#268](#268)
Bug Fixes ======= * **jsii:** use base interfaces for 'datatype' property ([#265](#265)) ([1c56902](1c56902)), closes [#264](#264) * **jsii:** use default jsx compiler options ([#261](#261)) ([bf1f586](bf1f586)), closes [aws/aws-cdk#830](aws/aws-cdk#830) * match behavioral interface to 'I'-prefix ([#271](#271)) ([03103f3](03103f3)) * require distinct argument and property names ([#272](#272)) ([4d2f268](4d2f268)), closes [#268](#268)
If an interface inherits from a non-datatype interface, it should no
longer be classified as a datatype interface itself.
Making this change requires that information about the base classes
has already been determined, so I introduced an ordering mechanism
for 'deferred's.
Fixes #264.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.