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

feat: Add InlineNodejsFunction to support in-project lambda handlers. #8

Closed
wants to merge 0 commits into from

Conversation

michanto
Copy link

@michanto michanto commented Apr 5, 2024

Reason for this change

Inline code would be more useful if customers could write lambda handlers in their CDK projects in the language of their choice, and deploy them. This is possible in interpreted languages such as Typescript and Python. This change makes writing inline lambdas easier by utilizing the Nodejs compiler to generate javascript, rather than the user entering an uncompiled string as code.

Checklist

  • My code Adheres to the CONTRIBUTING GUIDE.

@mrgrain
Copy link

mrgrain commented Apr 5, 2024

Hey congrats to your first PR!

I'm not an official reviewer, but I have the pretty strong view that we can't (or shouldn't) bundle esbuild in a jsii library. Esbuild is basically a go binary with a JavaScript interface and it does some pretty involved stuff to install the binary with the correct architecture.

For mrgrain/cdk-esbuild I went through this and couldn't get it to work. I ended up with a rather involved and silly system to automatically install esbuild when needed.

* Otherwise default to NODEJS_18_X.
*/
function getRuntime(scope: Construct, props: InlineNodejsFunctionProps): Runtime {
const defaultRuntime = FeatureFlags.of(scope).isEnabled(LAMBDA_NODEJS_USE_LATEST_RUNTIME)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we want to inherit this kind of baggage from aws-cdk-lib

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I did was go through NodejsFunction in the CDK and pulled out any code I thought I needed to get functional parity with NodejsFunction. I'm totally fine removing this logic. It wasn't easy to test (as you see).

@michanto
Copy link
Author

michanto commented Apr 5, 2024

Yeah, I was pretty sure that would be the first thing pointed out. I'm not sure what the correct way to handle this is.

I know that CDK has a way of using esbuild that is not the normal, and I wasn't sure if I should go that route, this route, or put ESBUILD in devDependencies. I certainly want to be able to test this code both with and without esbuild

Should I move esbuild to devDependencies?

@mrgrain
Copy link

mrgrain commented Apr 5, 2024

Finally while I'm not a maintainer of this package here and I'm obviously biased towards my own package, there is a question if this library does want to concern itself with problems that have already been solved by the community.

@mrgrain
Copy link

mrgrain commented Apr 5, 2024

Yeah, I was pretty sure that would be the first thing pointed out. I'm not sure what the correct way to handle this is.

I know that CDK has a way of using esbuild that is not the normal, and I wasn't sure if I should go that route, this route, or put ESBUILD in devDependencies. I certainly want to be able to test this code both with and without esbuild

Should I move esbuild to devDependencies? I will contact you on slack to see what kinds of errors you had.

You can have a look at how upstream CDK does it. It's defaulting to docker, with a shortcut if esbuild is found locally. A devDependency would probably work to test both cases.

@dbartholomae
Copy link
Contributor

@michanto Thanks for your contribution here! We might currently still be a bit slow in accepting PRs as we are still figuring out the best ways to work together, so please be patient with us :)

there is a question if this library does want to concern itself with problems that have already been solved by the community.

That's a good question, @mrgrain. I personally feel like one of the goals of this library could also be to make existing constructs from the community more visible and allow consumers to trust simpler processes (e.g. they might get one-time approval from their architects to use this library instead of needing approval for every individual package) - but I don't think we explicitly discussed this yet.

@dbartholomae
Copy link
Contributor

dbartholomae commented Apr 15, 2024

there is a question if this library does want to concern itself with problems that have already been solved by the community.

That's a good question, @mrgrain. I personally feel like one of the goals of this library could also be to make existing constructs from the community more visible and allow consumers to trust simpler processes (e.g. they might get one-time approval from their architects to use this library instead of needing approval for every individual package) - but I don't think we explicitly discussed this yet.

I've opened an issue to discuss this fundamental question further: #13

@EYssel
Copy link
Contributor

EYssel commented Apr 15, 2024

Hi @dbartholomae

The link provided is not accessible for me.

Is it maybe a private repo?

@dbartholomae
Copy link
Contributor

Yes, sorry, I corrected the link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants