search

aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.38k stars 3.79k forks source link

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

Open garysassano opened 4 months ago

garysassano commented 4 months ago

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 commented 4 months ago

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

garysassano commented 4 months ago

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 commented 4 months ago

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.

garysassano commented 4 months ago

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 commented 4 months ago

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