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/aws-lambda-nodejs: esbuildArgs alias must use : instead of = #25385

Closed
abaschen opened this issue May 1, 2023 · 4 comments · Fixed by #29167 · May be fixed by NOUIY/aws-solutions-constructs#98 or NOUIY/aws-solutions-constructs#99
Labels
@aws-cdk/aws-lambda-nodejs bug This issue is a bug. effort/small Small work item – less than a day of effort p1

Comments

@abaschen
Copy link

abaschen commented May 1, 2023

Describe the bug

I want to reference libs in my layers with import { somefunc } from '@layer/somelib';.
When setting the alias build argument for esbuild, it throws an error :

const opts = {
            bundling: {
                externalModules: ['@aws-sdk/*', 'aws-lambda', '@layer/*'],
                esbuildArgs: {
                    '--alias': '@layer=',
                },
            },
        };

It will generate a bundling step like this:

esbuild --bundle src/index.js  --outfile=dist/index.js --alias=@layer=
✘ [ERROR] Invalid build flag: "--alias=@layer="

  Use "--alias:@layer=" instead of "--alias=@layer=". Flags that can be re-specified multiple times
  use ":" instead of "=".

By replacing the = with : it solves the issue

Expected Behavior

esbuildArgs key/value needs : when it can be re-specified.

Current Behavior

esbuildArgs['--alias'] is using = which leads to a bundle error.

Full stack:

    at AssetStaging.bundle (node_modules/aws-cdk-lib/core/lib/asset-staging.js:2:603)
    at AssetStaging.stageByBundling (node_modules/aws-cdk-lib/core/lib/asset-staging.js:1:4540)
    at stageThisAsset (node_modules/aws-cdk-lib/core/lib/asset-staging.js:1:1901)
    at Cache.obtain (node_modules/aws-cdk-lib/core/lib/private/cache.js:1:242)
    at new AssetStaging (node_modules/aws-cdk-lib/core/lib/asset-staging.js:1:2296)
    at new Asset (node_modules/aws-cdk-lib/aws-s3-assets/lib/asset.js:1:736)
    at AssetCode.bind (node_modules/aws-cdk-lib/aws-lambda/lib/code.js:1:4628)
    at new Function (node_modules/aws-cdk-lib/aws-lambda/lib/function.js:1:2829)
    at new NodejsFunction (node_modules/aws-cdk-lib/aws-lambda-nodejs/lib/function.js:1:1171)
    at new MyConstruct (stack/MyConstruct .ts:18:9)

Reproduction Steps

CDK:

new NodejsFunction(this, 'test-function', {
            entry: 'src/functions/test/index.ts',
            architecture: Architecture.ARM_64,
            runtime: Runtime.NODEJS_18_X,
            memorySize: 128,
            tracing: Tracing.ACTIVE,
            handler: 'index.handler',
            timeout: Duration.seconds(30),
            retryAttempts: 0,
            logRetention: RetentionDays.ONE_DAY,
            bundling: {
                minify: process.env.NODE_ENV === 'production',
                banner: 'import { createRequire } from \'module\'; const require = createRequire(import.meta.url);',
                mainFields: ['module', 'main'],
                target: 'node18',
                externalModules: ['@aws-sdk/*', 'aws-lambda', '@layer/*'],
                esbuildArgs: {
                    '--alias': '@layer=',
                },
                format: OutputFormat.ESM,
            },
        });

Lambda

// src/functions/test/index.ts
import { ping } from '@layer/echo';

Layer

// dist/layers/echo/index.js
export{console.log as ping};

Possible Solution

esbuildArgs key/value needs : when it can be re-specified; from documentation:

CLI: If you are using the command-line API, it may be helpful to know that the flags come in one of three forms: --foo, --foo=bar, or --foo:bar. The form --foo is used for enabling boolean flags such as --minify, the form --foo=bar is used for flags that have a single value and are only specified once such as --platform=, and the form --foo:bar is used for flags that have multiple values and can be re-specified multiple times such as --external:.

Additional Information/Context

No response

CDK CLI Version

2.76.0 (build 78c411b)

Framework Version

No response

Node.js Version

Node.js v19.8.1

OS

WSL2 Ubuntu

Language

Typescript

Language Version

Typescript (5.0.4)

Other information

No response

@abaschen abaschen added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 1, 2023
@abaschen
Copy link
Author

abaschen commented May 1, 2023

Using this argument instead will solve the issue:

esbuildArgs: {
    '--alias:@layer': 'my/new/path',
},

It will generate the proper --alias:@layer=my/new/path argument

In my case :

  externalModules: ['@aws-sdk/*', 'aws-lambda', '@layer/*', '/opt/nodejs/node_modules/*'],
  esbuildArgs: {
    '--alias:@layer': '/opt/nodejs/node_modules',
  },

@peterwoodworth
Copy link
Contributor

You're right, we don't have any logic for handling this. The code that currently handles this is found here

function toCliArgs(esbuildArgs: { [key: string]: string | boolean }): string {
const args = [];
for (const [key, value] of Object.entries(esbuildArgs)) {
if (value === true || value === '') {
args.push(key);
} else if (value) {
args.push(`${key}="${value}"`);
}
}
return args.join(' ');
}

Looking at the documentation, it doesn't appear your workaround would work for all use cases, is that right?

@peterwoodworth peterwoodworth added p1 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 May 1, 2023
@abaschen
Copy link
Author

abaschen commented May 3, 2023

It's just a workaround and won't work for empty value but besides that seems like an OK'ish effortless solution.


to fix it though we would need to go through all of them. but by default we could keep the '=' sign :

   const repeatableArgs = ['alias'];
   for (const [key, value] of Object.entries(esbuildArgs)) { 
     if (value === true || value === '') { 
       args.push(key); 
     } else if (value) { 
       const separator = repeatableArgs.includes(key) ? ':' : '=';
       args.push(`${key}${separator}"${value}"`); 
     } 
   } 

Something like that and fill the repeatableArgs with all the correct keys.

Or get it from the types https://github.com/evanw/esbuild/blob/main/lib/shared/types.ts it seems that any types with Record or string[] would be with ':' ?

@mergify mergify bot closed this as completed in #29167 Mar 8, 2024
mergify bot pushed a commit that referenced this issue Mar 8, 2024
…d keys (#29167)

### Issue # (if applicable)
Closes #25385 

### Reason for this change
This PR fixes a bug in CDK where CDK does not take into account re-specified keys while doing bundling. The CLI supports flags in one of three forms: `--foo`, `--foo=bar`, or `--foo:bar`. However, the `--foo:bar` form was not initially supported. 
With `--foo:bar`, users can now specify flags that have multiple values and can be re-specified multiple times.

### Description of changes
The code has a list of keys that can be re-specified multiple times. While assigning the flags it checks whether the key is among the list of re-specified keys, if yes, it specifies the flag with `:`.

### Description of how you validated changes
The PR includes unit test and integration test both to validate the changes.

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

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

github-actions bot commented Mar 8, 2024

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

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 p1
Projects
None yet
2 participants