fix(jsii): build succeeds using Omit<T, K> #1708
Conversation
When using the `Omit<T, K>` helper type (a mapped type) on an exported API, `jsii` should detect this and fail, instead of allowing the application to compile. An additional bug caused use of this type to result in a successful exit code from `jsii`, while the `.jsii` file was never written. This has also been fixed. Fixes #1707
RomainMuller
added
bug
| @@ -1587,7 +1628,7 @@ export class Assembler implements Emitter { | |||
| for (const member of declaringType.getProperties()) { | |||
| if ( | |||
| !(declaringType.symbol.getDeclarations() ?? []).find( | |||
| (decl) => decl === member.valueDeclaration.parent, | |||
| (decl) => decl === member.valueDeclaration?.parent, | |||
There was a problem hiding this comment.
The ? here is due to valueDeclaration being nullable (despite the type declarations do not mention it).
| @@ -246,6 +246,7 @@ export class Compiler implements Emitter { | |||
| diagnostics.push(...assmEmit.diagnostics); | |||
| } catch (e) { | |||
| LOG.error(`Error during type model analysis: ${e}\n${e.stack}`); | |||
| hasErrors = true; | |||
There was a problem hiding this comment.
This previously caused exceptions thrown from the assembler to be "swallowed" (after logging to STDERR) without necessarily causing the process to exit with non-0 (hasErrors could be false when this catch block triggers!).
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| for (const clause of clauses) { | ||
| for (const node of clause.types) { | ||
| const parentType = this._typeChecker.getTypeAtLocation(node); | ||
| // For some reason, we cannot trust parentType.isClassOrInterface() |
There was a problem hiding this comment.
Expand a bit with an example of when it has lost your trust?
There was a problem hiding this comment.
Basically, imported types respond false to all is* methods. :(
| `Illegal "${clauseType(clause.token)}" value for an exported API: ${ | ||
| ts.SyntaxKind[badDecl.kind] | ||
| }`, |
There was a problem hiding this comment.
What does this error look like the neg.omit.*.ts?
Make sure it's easy to understand for an external contributor to the CDK.
There was a problem hiding this comment.
Illegal "extends" value for an exported API: Omit<SomeInterface, 'somefield'>
I'd have wanted to make it more specific (mapped/utility types are not allowed here), however the type checker does not really give me a way to do this with confidence...
RomainMuller
removed
the
pr/do-not-merge
|
Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it! |
|
Merging (with squash)... |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Merging (with squash)... |
When using the
Omit<T, K>helper type (a mapped type) on an exportedAPI,
jsiishould detect this and fail, instead of allowing theapplication to compile.
An additional bug caused use of this type to result in a successful exit
code from
jsii, while the.jsiifile was never written. This was causedby the mapped types symbols not having a
valueDeclaration, triggeringan attempt to read
undefined.parent, which threw an exception caughtby the
Compilerclass, but not signaled back to the caller.Fixes #1707
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.