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.

Sign up for GitHub

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
merged 12 commits into from
Mar 10, 2022
Merged

feat(aws-lambda-nodejs): support additional esbuild configurations #17788

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e22dcf8
feat: Added `esbuildArgs` prop and updated test
ryparker Dec 1, 2021
deed531
Added example to docs
ryparker Dec 1, 2021
ab98bae
Updated README
ryparker Dec 1, 2021
9401d81
Using `Object.entries()` and wrapped arg value in double quotes
ryparker Dec 10, 2021
5d13ddf
Merge branch 'master' into feat-aws-lambda-nodejs-support-additional-…
ryparker Dec 10, 2021
8e974ce
Merge branch 'master' into feat-aws-lambda-nodejs-support-additional-…
ryparker Mar 9, 2022
74d6a11
Update `esbuildArgs ` key: value type to include boolean
ryparker Mar 9, 2022
c8ea7ee
Updated `esbuildArgs` type doc example to configure `—log-limit`
ryparker Mar 9, 2022
e8ca3c7
Merge branch 'master' into feat-aws-lambda-nodejs-support-additional-…
ryparker Mar 9, 2022
3e5d859
fix tests and replaced example uses of `—keep-names` with `—splitting`
ryparker Mar 9, 2022
1784f0d
fix(README): JSON commas
ryparker Mar 9, 2022
ef2601a
Merge branch 'master' into feat-aws-lambda-nodejs-support-additional-…
ryparker Mar 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions packages/@aws-cdk/aws-lambda-nodejs/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ Docker container even if `esbuild` is available in your environment. This can be

## Configuring `esbuild`

The `NodejsFunction` construct exposes some [esbuild options](https://esbuild.github.io/api/#build-api)
The `NodejsFunction` construct exposes [esbuild options](https://esbuild.github.io/api/#build-api)
via properties under `bundling`:

```ts
Expand All @@ -191,6 +191,9 @@ new lambda.NodejsFunction(this, 'my-handler', {
banner: '/* comments */', // requires esbuild >= 0.9.0, defaults to none
footer: '/* comments */', // requires esbuild >= 0.9.0, defaults to none
charset: lambda.Charset.UTF8, // do not escape non-ASCII characters, defaults to Charset.ASCII
esbuildArgs: { // pass additional arguments to esbuild
"--main-fields": "module,main"
},
},
});
```
Expand Down Expand Up @@ -251,7 +254,7 @@ new lambda.NodejsFunction(this, 'my-handler', {
});
```

Note: A [`tsconfig.json` file](https://www.typescriptlang.org/docs/handbook/tsconfig-json.html) is required
Note: A [`tsconfig.json` file](https://www.typescriptlang.org/docs/handbook/tsconfig-json.html) is required

## Customizing Docker bundling

Expand Down
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-lambda-nodejs/lib/bundling.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 [email protected] ....

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

];

let depsCommand = '';
Expand Down
22 changes: 21 additions & 1 deletion packages/@aws-cdk/aws-lambda-nodejs/lib/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,12 +202,32 @@ export interface BundlingOptions {
*/
readonly esbuildVersion?: string;

/**
* Build arguments to pass into esbuild.
*
* For example, to add the [--main-fields](https://esbuild.github.io/api/#main-fields) flag:
*
* ```text
* new NodejsFunction(scope, id, {
* ...
* bundling: {
* esbuildArgs: {
* "--main-fields": "module,main"
* }
* }
* });
* ```
*
* @default - no additional build arguments are passed
*/
readonly esbuildArgs?: { [key: string]: string };

/**
* Build arguments to pass when building the bundling image.
*
* @default - no build arguments are passed
*/
readonly buildArgs?: { [key:string] : string };
readonly buildArgs?: { [key: string]: string };

/**
* Force bundling in a Docker container even if local bundling is
Expand Down
6 changes: 6 additions & 0 deletions packages/@aws-cdk/aws-lambda-nodejs/test/bundling.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,11 @@ test('esbuild bundling with esbuild options', () => {
'process.env.NUMBER': '7777',
'process.env.STRING': JSON.stringify('this is a "test"'),
},
esbuildArgs: {
'--main-fields': 'module,main',
'--log-limit': '0',
'--resolve-extensions': '.ts,.js',
},
});

// Correctly bundles with esbuild
Expand All @@ -224,6 +229,7 @@ test('esbuild bundling with esbuild options', () => {
'--log-level=silent --keep-names --tsconfig=/asset-input/lib/custom-tsconfig.ts',
'--metafile=/asset-output/index.meta.json --banner:js="/* comments */" --footer:js="/* comments */"',
'--charset=utf8',
'--main-fields=module,main --log-limit=0 --resolve-extensions=.ts,.js',
].join(' '),
],
}),
Expand Down