-
Notifications
You must be signed in to change notification settings - Fork 4k
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(aws-lambda-nodejs): support additional esbuild configurations #17788
feat(aws-lambda-nodejs): support additional esbuild configurations #17788
Conversation
@@ -204,6 +204,7 @@ export class Bundling implements cdk.BundlingOptions { | |||
...this.props.banner ? [`--banner:js=${JSON.stringify(this.props.banner)}`] : [], | |||
...this.props.footer ? [`--footer:js=${JSON.stringify(this.props.footer)}`] : [], | |||
...this.props.charset ? [`--charset=${this.props.charset}`] : [], | |||
...this.props.esbuildArgs ? [Object.keys(this.props.esbuildArgs).map((key) => `${key}=${this.props.esbuildArgs![key]}`).join(' ')] : [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...this.props.esbuildArgs ? [Object.keys(this.props.esbuildArgs).map((key) => `${key}=${this.props.esbuildArgs![key]}`).join(' ')] : [], | |
...this.props.esbuildArgs ? [Object.entries(this.props.esbuildArgs).map(([key, value]) => `${key}=${value}`).join(' ')] : [], |
+ should value
be enclosed with double quotes? ${key}="${value}"
to cover args where value is a path that can contain whitespaces? or should the user take care of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah and what about args that don't have values like --keep-names
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestions, I'll get those implemented. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah and what about args that don't have values like --keep-names?
@jogold because the type for esbuildArgs
is an object we can always expect a value being assigned to each key. I think it's reasonable to assume that the user will need to pass a truthy value into the value of those boolean args.
i.g.
...
esbuildArgs: {
'--log-limit': '0',
'--resolve-extensions': '.ts,.js',
'--splitting': true,
},
I'll update the type to
readonly esbuildArgs?: { [key: string]: string | boolean };
I added an example of this to the README example and updated tests for clarification. LMK if you think this is acceptable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But esbuild
doesn't accept this syntax in the CLI...
> error: Invalid build flag: "--splitting=true"
You need to update the logic to treat the Booleans differently (output only the key)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not able to reproduce this error. See my tests below:
$ esbuild src/* --bundle --outdir=output/splitting-novalue --format=esm --splitting
output/splitting-novalue/app.js 133.9kb
output/splitting-novalue/chunk-BJ7SH3MN.js 131.7kb
output/splitting-novalue/chunk-IZI55ZEC.js 447b
output/splitting-novalue/chunk-QND2VRQK.js 442b
output/splitting-novalue/GreenSpinner.js 138b
output/splitting-novalue/BlueSpinner.js 136b
output/splitting-novalue/Spinner.js 98b
$ esbuild src/* --bundle --outdir=output/splitting-withvalue-true --format=esm --splitting=true
output/splitting-withvalue-true/app.js 133.9kb
output/splitting-withvalue-true/chunk-BJ7SH3MN.js 131.7kb
output/splitting-withvalue-true/chunk-IZI55ZEC.js 447b
output/splitting-withvalue-true/chunk-QND2VRQK.js 442b
output/splitting-withvalue-true/GreenSpinner.js 138b
output/splitting-withvalue-true/BlueSpinner.js 136b
output/splitting-withvalue-true/Spinner.js 98b
$ esbuild src/* --bundle --outdir=output/splitting-withvalue-false --format=esm --splitting=false
output/splitting-withvalue-false/app.js 266.0kb
output/splitting-withvalue-false/GreenSpinner.js 132.0kb
output/splitting-withvalue-false/BlueSpinner.js 132.0kb
output/splitting-withvalue-false/Spinner.js 131.7kb
$ esbuild src/* --bundle --outdir=output/splitting-withvalue-wrapped-true --format=esm --splitting="true"
output/splitting-withvalue-wrapped-true/app.js 133.9kb
output/splitting-withvalue-wrapped-true/chunk-BJ7SH3MN.js 131.7kb
output/splitting-withvalue-wrapped-true/chunk-IZI55ZEC.js 447b
output/splitting-withvalue-wrapped-true/chunk-QND2VRQK.js 442b
output/splitting-withvalue-wrapped-true/GreenSpinner.js 138b
output/splitting-withvalue-wrapped-true/BlueSpinner.js 136b
output/splitting-withvalue-wrapped-true/Spinner.js 98b
$ esbuild src/* --bundle --outdir=output/splitting-withvalue-wrapped-false --format=esm --splitting="false"
output/splitting-withvalue-wrapped-false/app.js 266.0kb
output/splitting-withvalue-wrapped-false/GreenSpinner.js 132.0kb
output/splitting-withvalue-wrapped-false/BlueSpinner.js 132.0kb
output/splitting-withvalue-wrapped-false/Spinner.js 131.7kb
✨ Done in 1.04s.
Could you share the command you're using to get that "Invalid build flag" error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK it works with esbuild
>= 0.14.24 published 8 days ago.
You can try with npx [email protected] ...
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest this #19343
@nija-at any chance to review this PR? |
…esbuild-configurations
We added support for —main-fields so I replaced the example with —log-limit which we do not currently support.
…esbuild-configurations
…esbuild-configurations
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
…ws#17788) ## Summary This PR adds support for passing in any additional esbuild args that are not already exposed in the `NodejsFunction` API. With this change a user should be able to add an esbuild option such as [`--log-limit`](https://esbuild.github.io/api/#log-limit): ```ts new NodejsFunction(scope, id, { ... bundling: { esbuildArgs: { "--log-limit": "0" } } }) ``` Resolves: aws#17768 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Summary
This PR adds support for passing in any additional esbuild args that are not already exposed in the
NodejsFunction
API.With this change a user should be able to add an esbuild option such as
--log-limit
:Resolves: #17768
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license