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(aws-lambda-nodejs): support additional esbuild configurations #17788

Merged

Conversation

ryparker
Copy link
Contributor

@ryparker ryparker commented Dec 1, 2021

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:

new NodejsFunction(scope, id, {
  ...
  bundling: {
    esbuildArgs: {
      "--log-limit": "0"
    }
  }
})

Resolves: #17768


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@gitpod-io
Copy link

gitpod-io bot commented Dec 1, 2021

@ryparker ryparker requested a review from nija-at December 1, 2021 01:25
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Dec 1, 2021
@ryparker ryparker self-assigned this Dec 1, 2021
@@ -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(' ')] : [],
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
...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?

Copy link
Contributor

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?

Copy link
Contributor Author

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!

Copy link
Contributor Author

@ryparker ryparker Mar 9, 2022

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.

Copy link
Contributor

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)

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'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?

Copy link
Contributor

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 esbuild@0.14.23 ....

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest this #19343

packages/@aws-cdk/aws-lambda-nodejs/lib/types.ts Outdated Show resolved Hide resolved
@alimek
Copy link

alimek commented Feb 25, 2022

@nija-at any chance to review this PR?

@ryparker ryparker removed the request for review from nija-at March 9, 2022 23:46
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 1784f0d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@ryparker ryparker added p2 feature-request A feature should be added or improved. labels Mar 10, 2022
@mergify
Copy link
Contributor

mergify bot commented Mar 10, 2022

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

@mergify mergify bot merged commit ab313a4 into master Mar 10, 2022
@mergify mergify bot deleted the feat-aws-lambda-nodejs-support-additional-esbuild-configurations branch March 10, 2022 01:18
TheRealAmazonKendra pushed a commit to TheRealAmazonKendra/aws-cdk that referenced this pull request Mar 11, 2022
…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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. feature-request A feature should be added or improved. p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(aws-lambda-nodejs): support additional esbuild configurations(main-fields)
6 participants