-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
fix(aws-lambda): java - invalid cast for inline LambdaRuntime members #505
Conversation
TypeScript uses inference to determine if a cast to an interface is legal but strongly-typed languages like Java require that the down-casted class will explicitly implement the interface. The JavaRuntime class has static members that are down-casted from JavaRuntime to a set of interfaces, to allow strong-typing of properties for various Lambda use cases. These down-casts fail in e.g. Java because JavaRuntime doesn't implement these interfaces explicitly. We should add a compile-time check in jsii for such a use case. Fixes #504
@@ -9,18 +9,18 @@ export interface LambdaRuntimeProps { | |||
/** | |||
* Lambda function runtime environment. | |||
*/ | |||
export class LambdaRuntime { | |||
export class LambdaRuntime implements InlinableLambdaRuntime, InlinableJavascriptLambdaRuntime { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we just get rid of these interfaces? Nothing takes them as arguments, Lambda
just takes a LambdaRuntime
argument, and since LambdaRuntime
already exposes the supportsInlineCode
property that code is looking for, we're already done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is currently used by the (soon to be removed) InlineJavaScriptLambda
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to do a bit of overhaul on Lambda soon, but in the meantime, I just want to unblock Java users from using Lambda.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case I would feel better if you turned InlineJavaScriptLambda
into a class and used the pattern I described in the other comments. That restores the type safety and still compiles properly:
public static get NodeJS(): InlinableJavascriptLambdaRuntime {
return new InlinableJavascriptLambdaRuntime('nodejs');
}
I will approve in any case; do what you deem best.
public static readonly NodeJS = new LambdaRuntime('nodejs', { supportsInlineCode: true }) as InlinableJavascriptLambdaRuntime; | ||
// Using ``as InlinableLambdaRuntime`` because that calss cannot be defined just yet | ||
// Using ``as InlinableLambdaRuntime`` because that class cannot be defined just yet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't know if this still matters since we should probably just get rid of the interfaces, but if this crops up again, the way to solve it is thusly:
public static get NodeJS43(): InlinableJavascriptLambda {
return new LambdaRuntime(...);
}
I.e., use a getter to temporally decouple type declaration from returned type. Types can use forward references, instantiations can't (because JavaScript).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, InlinableJavascriptLambda
was probably intended to be a class, but because of the issue of getting it to declare properly we made it an interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So intended was probably:
public static get NodeJS43(): InlinableJavascriptLambda {
return new InlinableJavascriptLambda(...);
}
But anyway, a concrete LambdaRuntime
class without interfaces probably suffices.
TypeScript uses inference to determine if a cast to an interface is legal but strongly-typed languages like Java require that the down-casted class will explicitly implement the interface.
The JavaRuntime class has static members that are down-casted from JavaRuntime to a set of interfaces, to allow strong-typing of properties for various Lambda use cases. These down-casts fail in e.g. Java because JavaRuntime doesn't implement these interfaces explicitly.
We should add a compile-time check in jsii for such a use case (aws/jsii#140)
Fixes #504
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.