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

(aws-cdk-lib/aws-lambda-nodejs BundlingOptions): platform property should default to node when not specified #29290

Open
garysassano opened this issue Feb 28, 2024 · 5 comments
Labels
@aws-cdk/aws-lambda-nodejs bug This issue is a bug. effort/small Small work item – less than a day of effort p3

Comments

@garysassano
Copy link

garysassano commented Feb 28, 2024

Describe the bug

The platform property in BundlingOptions interface has wrong information.

image

According to esbuild API docs, the default value is browser when no platform is specified. For this reason, we should change the default value to node for the BundlingOptions of the NodejsFunction resource.

Expected Behavior

see above

Current Behavior

see above

Reproduction Steps

see above

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.130.0

Framework Version

No response

Node.js Version

20.10.0

OS

Ubuntu 22.04.3 LTS

Language

TypeScript

Language Version

No response

Other information

No response

@garysassano garysassano added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 28, 2024
@garysassano
Copy link
Author

Actually, I think the --platform node option should be added to the default esbuildArgs.

@garysassano
Copy link
Author

garysassano commented Feb 28, 2024

I'm quite confused. If I check any property within NodejsFunction.bundling, they are of type BundlingOptions:

image

But if I check the platform property, it's of type DockerRunOptions.

image

This doesn't make any sense to me.

It's unclear if the platform property refers to an esbuild platform or a Docker build platform.

@pahud
Copy link
Contributor

pahud commented Feb 28, 2024

The --paltform prop is for docker image bundling and should be used with docker I believe. Maybe we should improve the doc for this prop.

@pahud pahud added p2 effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Feb 28, 2024
@garysassano
Copy link
Author

Mixing esbuild and Docker properties within the same interface seems to cause confusion. It might have been preferable to exclude esbuild properties entirely, relegating them to the esbuildArgs option instead. This approach would have ensured a clear distinction of responsibilities. Currently, the same outcome can be achieved either by directly setting certain properties or by including them in esbuildArgs.

You can also see from recent articles that there's ongoing confusion regarding the platform property being interpreted as an esbuild platform instead of a Docker build platform.

I also believe that the default setting for esbuildArgs should be { "--platform": "node" }. Without this, most users might end up targeting the browser platform, which is obviously not appropriate for a NodejsFunction.

A further improvement might be to add a property named esbuildPlatform to the BundlingOptions interface. This would clearly signal to users that this is distinct from the existing platform property, potentially reducing confusion. With the platform keyword triggering autocomplete suggestions in IDEs, this change could help users more easily avoid mistakes. Naturally, the default value for esbuildPlatform would be set to node.

@garysassano
Copy link
Author

Upon further examination, it appears that platform: node is actually already set as the default option for esbuild,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-lambda-nodejs bug This issue is a bug. effort/small Small work item – less than a day of effort p3
Projects
None yet
Development

No branches or pull requests

2 participants