-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Add support for Node8.10 Lambda runtime #187
Conversation
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.
Is it supported by CloudFormation?
It is. But not for inline code -_- |
Perhaps you can attend to #188 and model this a bit better, no? |
Fixes #203 |
Also, re-model LambdaRuntime as a class to allow customers to use runtimes that are not yet part of the modelled list, optionally with support for inline code. The `InlinableLambdaRuntime` class is mostly provided to allow for a strictly typed interface in `InlineJavaScriptLambda`. Fixes #187 Fixes #188 Fixes #203
Did the re-model-enum-as-class, and removed the NodeJS8.10 from the inline-able list. |
/** Whether the ``ZipFile`` (aka inline code) property can be used with this runtime. */ | ||
public readonly supportsInlineCode: boolean; | ||
|
||
constructor(name: string, supportsInlineCode: boolean = false) { |
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.
Use props
instead of a positional argument. I suspect we are going to extend this type as we introduce support for runtime values (and generally it will be more readable and future-compatible).
* A ``LambdaRuntime`` that can be used for inlining JavaScript. | ||
*/ | ||
export interface InlinableJavascriptLambdaRuntime extends InlinableLambdaRuntime { | ||
readonly name: string; |
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.
Why are these required here? They are inherited from InlinableLambdaRuntime
no?
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.
They are. I was trying to stop tslint from complaining in an elegant (aka not // tslint:disable-next-line:...
) way. But that causes the Java code generation to be broken (rightfully so, I think).
public static readonly NodeJS810 = new LambdaRuntime('nodejs8.10'); | ||
public static readonly Java8 = new LambdaRuntime('java8'); | ||
// Using ``as InlinableLambdaRuntime`` because that calss cannot be defined just yet | ||
public static readonly Python27 = new LambdaRuntime('python2.7', true) as InlinableLambdaRuntime; |
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.
Why not make InlinablePythonLambdaRuntime
why you are at it?
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.
There's no interest in it... The JS one is used by the InlineJavaScriptLambda
class, which takes a literal javascript function and toString()
's it to the inline code. I have concerns over how this can only be used when authoring TypeScript/Javascript applications, but since the class is there I retained the functionality.
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.
You are right. Will probably deprecate this class soon when we have better support for runtime code.
Also, re-model LambdaRuntime as a class to allow customers to use runtimes that are not yet part of the modelled list, optionally with support for inline code. The `InlinableLambdaRuntime` class is mostly provided to allow for a strictly typed interface in `InlineJavaScriptLambda`. Fixes #188 Fixes #203
4b86514
to
f7a5c3b
Compare
Also make it the defaul for the
InlineJavaScriptLambda
.By submitting this pull request, I confirm that my contribution is made under
the terms of the beta license.