Skip to content
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

Merged
merged 1 commit into from
Jul 2, 2018

Conversation

RomainMuller
Copy link
Contributor

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.

@RomainMuller RomainMuller requested a review from eladb June 28, 2018 17:09
Copy link
Contributor

@eladb eladb left a 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?

@RomainMuller
Copy link
Contributor Author

It is. But not for inline code -_-

@eladb
Copy link
Contributor

eladb commented Jun 29, 2018

Perhaps you can attend to #188 and model this a bit better, no?

@eladb
Copy link
Contributor

eladb commented Jun 30, 2018

Fixes #203

RomainMuller added a commit that referenced this pull request Jul 2, 2018
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
@RomainMuller
Copy link
Contributor Author

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) {
Copy link
Contributor

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;
Copy link
Contributor

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?

Copy link
Contributor Author

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;
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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
@RomainMuller RomainMuller merged commit f7a5c3b into master Jul 2, 2018
@RomainMuller RomainMuller deleted the rmuller/add-lambda-node8.10 branch July 2, 2018 09:57
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants