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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Aug 6, 2018
Merged

fix(aws-lambda): java - invalid cast for inline LambdaRuntime members #505

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions packages/@aws-cdk/aws-lambda/lib/runtime.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,18 @@ export interface LambdaRuntimeProps {
/**
* Lambda function runtime environment.
*/
export class LambdaRuntime {
export class LambdaRuntime implements InlinableLambdaRuntime, InlinableJavascriptLambdaRuntime {
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

Copy link
Contributor

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

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).

Copy link
Contributor

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?

Copy link
Contributor

@rix0rrr rix0rrr Aug 6, 2018 •
edited
Loading

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.

public static readonly NodeJS43 = new LambdaRuntime('nodejs4.3', { supportsInlineCode: true }) as InlinableJavascriptLambdaRuntime;
public static readonly NodeJS43Edge = new LambdaRuntime('nodejs4.3-edge');
// Using ``as InlinableLambdaRuntime`` because that calss cannot be defined just yet
// Using ``as InlinableLambdaRuntime`` because that class cannot be defined just yet
public static readonly NodeJS610 = new LambdaRuntime('nodejs6.10', { supportsInlineCode: true }) as InlinableJavascriptLambdaRuntime;
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
// Using ``as InlinableLambdaRuntime`` because that class cannot be defined just yet
public static readonly Python27 = new LambdaRuntime('python2.7', { supportsInlineCode: true }) as InlinableLambdaRuntime;
// Using ``as InlinableLambdaRuntime`` because that calss cannot be defined just yet
// Using ``as InlinableLambdaRuntime`` because that class cannot be defined just yet
public static readonly Python36 = new LambdaRuntime('python3.6', { supportsInlineCode: true }) as InlinableLambdaRuntime;
public static readonly DotNetCore1 = new LambdaRuntime('dotnetcore1.0');
public static readonly DotNetCore2 = new LambdaRuntime('dotnetcore2.0');
Expand All @@ -47,7 +47,7 @@ export class LambdaRuntime {
*/
export interface InlinableLambdaRuntime {
readonly name: string;
readonly supportsInlineCode: true;
readonly supportsInlineCode: boolean;
}

/**
Expand Down