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-lambda-nodejs): Use esbuild API instead of CLI for bundling #18470

Open
azatoth opened this issue Jan 17, 2022 · 19 comments · May be fixed by #30196
Open

(aws-lambda-nodejs): Use esbuild API instead of CLI for bundling #18470

azatoth opened this issue Jan 17, 2022 · 19 comments · May be fixed by #30196
Assignees
Labels
@aws-cdk/aws-lambda-nodejs effort/large Large work item – several weeks of effort feature-request A feature should be added or improved. p1

Comments

@azatoth
Copy link
Contributor

azatoth commented Jan 17, 2022

What is the problem?

If a dependency has a .node dependency, for example ssh2 in node_modules/ssh2/lib/protocol/crypto.js has

binding = require('./crypto/build/Release/sshcrypto.node');,

this will have esbuild fail the bundling with

error: No loader is configured for ".node" files: ../node_modules/ssh2/lib/protocol/crypto/build/Release/sshcrypto.node.

In evanw/esbuild#1051 they provide a plugin, but sadly plugins can't be used with binary esbuild.

Reproduction Steps

Run npx cdk init app --language=typescript to create sample app

Add new NodejsFunction(this, 'testfunc', { entry: 'lib/testfunc/index.ts' }); to test stack

Add to file lib/testfunc/index.ts:

import { Client } from "ssh2";

const handler = async () => {
  const conn = new Client();

  conn.end();
};

export { handler };

Add to file lib/testfunc/package.json:

{
  "dependencies": {
    "ssh2": "^1.5.0"
  }
}

Run npm run cdk synth

What did you expect to happen?

It bundling the example code

What actually happened?

$ npm run cdk synth

> cdktest@0.1.0 cdk
> cdk "synth"

Bundling asset CdktestStack/testfunc/Code/Stage...
✘ [ERROR] No loader is configured for ".node" files: lib/testfunc/node_modules/cpu-features/build/Release/cpufeatures.node

    lib/testfunc/node_modules/cpu-features/lib/index.js:1:24:
      1 │ const binding = require('../build/Release/cpufeatures.node');
        ╵                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

✘ [ERROR] No loader is configured for ".node" files: lib/testfunc/node_modules/ssh2/lib/protocol/crypto/build/Release/sshcrypto.node

    lib/testfunc/node_modules/ssh2/lib/protocol/crypto.js:30:20:
      30 │   binding = require('./crypto/build/Release/sshcrypto.node');
         ╵                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

2 errors

/home/azatoth/tmp/cdktest/node_modules/aws-cdk-lib/core/lib/asset-staging.ts:395
      throw new Error(`Failed to bundle asset ${this.node.path}, bundle output is located at ${bundleErrorDir}: ${err}`);
            ^
Error: Failed to bundle asset CdktestStack/testfunc/Code/Stage, bundle output is located at /home/azatoth/tmp/cdktest/cdk.out/bundling-temp-43706a6374f4884c6246dca414315bc291a0eff7496c6eda3da7968ee92f4c47-error: Error: bash -c npx --no-install esbuild --bundle "/home/azatoth/tmp/cdktest/lib/testfunc/index.ts" --target=node14 --platform=node --outfile="/home/azatoth/tmp/cdktest/cdk.out/bundling-temp-43706a6374f4884c6246dca414315bc291a0eff7496c6eda3da7968ee92f4c47/index.js" --external:aws-sdk run in directory /home/azatoth/tmp/cdktest exited with status 1
    at AssetStaging.bundle (/home/azatoth/tmp/cdktest/node_modules/aws-cdk-lib/core/lib/asset-staging.ts:395:13)
    at AssetStaging.stageByBundling (/home/azatoth/tmp/cdktest/node_modules/aws-cdk-lib/core/lib/asset-staging.ts:243:10)
    at stageThisAsset (/home/azatoth/tmp/cdktest/node_modules/aws-cdk-lib/core/lib/asset-staging.ts:134:35)
    at Cache.obtain (/home/azatoth/tmp/cdktest/node_modules/aws-cdk-lib/core/lib/private/cache.ts:24:13)
    at new AssetStaging (/home/azatoth/tmp/cdktest/node_modules/aws-cdk-lib/core/lib/asset-staging.ts:159:44)
    at new Asset (/home/azatoth/tmp/cdktest/node_modules/aws-cdk-lib/aws-s3-assets/lib/asset.ts:72:21)
    at AssetCode.bind (/home/azatoth/tmp/cdktest/node_modules/aws-cdk-lib/aws-lambda/lib/code.ts:180:20)
    at new Function (/home/azatoth/tmp/cdktest/node_modules/aws-cdk-lib/aws-lambda/lib/function.ts:350:29)
    at new NodejsFunction (/home/azatoth/tmp/cdktest/node_modules/aws-cdk-lib/aws-lambda-nodejs/lib/function.ts:50:5)
    at new CdktestStack (/home/azatoth/tmp/cdktest/lib/cdktest-stack.ts:9:5)
Subprocess exited with error 1

CDK CLI Version

2.8.0 (build 8a5eb49)

Framework Version

No response

Node.js Version

v14.18.3

OS

Linux

Language

Typescript

Language Version

No response

Other information

No response

@azatoth azatoth added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 17, 2022
@jogold
Copy link
Contributor

jogold commented Jan 17, 2022

Does it work with this?

new NodejsFunction(this, 'testfunc', {
  entry: 'lib/testfunc/index.ts',
  bundling: {
    nodeModules: ['ssh2'],
  }
});

See https://github.com/aws/aws-cdk/tree/master/packages/%40aws-cdk/aws-lambda-nodejs#install-modules

@azatoth
Copy link
Contributor Author

azatoth commented Jan 18, 2022

Does it work with this?

new NodejsFunction(this, 'testfunc', {
  entry: 'lib/testfunc/index.ts',
  bundling: {
    nodeModules: ['ssh2'],
  }
});

See https://github.com/aws/aws-cdk/tree/master/packages/%40aws-cdk/aws-lambda-nodejs#install-modules

This would work as a workaround, though I wouldn't say its a solution to the problem.

@peterwoodworth peterwoodworth added p1 effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 and removed needs-triage This issue or PR still needs to be triaged. bug This issue is a bug. p1 effort/small Small work item – less than a day of effort labels Jan 19, 2022
@corymhall
Copy link
Contributor

@azatoth what are you looking for in a solution that isn't covered by what jogold suggested? That is the way we (aws-lambda-nodejs) support .node dependencies. It seems like the only way to change this is for esbuild to support it natively which is not something we have control over.

@peterwoodworth peterwoodworth added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jan 26, 2022
@github-actions
Copy link

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Jan 28, 2022
@azatoth
Copy link
Contributor Author

azatoth commented Jan 28, 2022

@azatoth what are you looking for in a solution that isn't covered by what jogold suggested? That is the way we (aws-lambda-nodejs) support .node dependencies. It seems like the only way to change this is for esbuild to support it natively which is not something we have control over.

I'm looking for a solution that actually bundles the dependencies instead of including them; Which is what I meant by using nodeModules to include them is a workaround and not a solution.

From what I've seen from esbuild is that they support this via plugins, but only if you run esbuild as code you can use plugins. So the natural solution would be for cdk to allow us to execute esbuild as code instead of as binary.

@github-actions github-actions bot removed closing-soon This issue will automatically close in 4 days unless further comments are made. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Jan 29, 2022
@corymhall
Copy link
Contributor

Ah ok I see what you are saying.

I am unassigning and marking this issue as p2, which means that we are unable to work on this immediately.

We use +1s to help prioritize our work, and are happy to revaluate this issue based on community feedback. You can reach out to the cdk.dev community on Slack to solicit support for reprioritization.

@corymhall corymhall changed the title (aws-lambda-nodejs): Bundling fails if dependency has .node modules (aws-lambda-nodejs): Add support for bundling .node modules Feb 4, 2022
@corymhall corymhall added the effort/large Large work item – several weeks of effort label Feb 4, 2022
@corymhall corymhall removed their assignment Feb 4, 2022
@shellscape
Copy link

In many cases, using the esbuild API instead of the CLI will open up the plugins configuration for ESBuild to the users.

@corymhall corymhall changed the title (aws-lambda-nodejs): Add support for bundling .node modules (aws-lambda-nodejs): Use esbuild API instead of CLI for bundling Aug 2, 2022
@shellscape
Copy link

Importing my comment from #21161 :

Tossing in another vote for using ESBuild via the API so we can leverage plugins. I quite literally cannot use format: esm with NodeJsFunction because of use of __dirname and the inability to use a plugin to intelligently replace __dirname in the bundle.

@ghost
Copy link

ghost commented Nov 7, 2022

Brainstorming, it seems like mostly what is needed is an option to override this command to use a custom build script: https://github.com/aws/aws-cdk/blob/v2.50.0/packages/%40aws-cdk/aws-lambda-nodejs/lib/bundling.ts#L180 and also to copy the custom build script to the Docker image when building via Docker...

For reference, the issue as seen from the esbuild side: evanw/esbuild#884

@ChristianMaehler
Copy link

I stumbled upon the same issue without knowing why. We had a running application, everything was fine, and one day it happens from one GIT commit to the next (the commit contained modified Jest tests, nothing with package.json). For me, this setting worked

new NodejsFunction(this, 'testfunc', {
  entry: 'lib/testfunc/index.ts',
  bundling: {
    loader: {
      '.node': 'text'
  }
});

It was more or less trial-and-error and choosing different types of loaders as listed in https://esbuild.github.io/content-types/#tsconfig-json . I tried "js" or "ts" as loader but it didn't work. 'text' did the job.

@VanTanev
Copy link

This issue was discussed on esbuild's side: evanw/esbuild#884

There is no way for esbuild to provide plugins support on the CLI, so it's up to AWS-CDK to allow us to configure esbuild through JS instead of through CLI alone.

In our case, we use @preconstruct/cli inside monorepo to resolve monorepo-internal dependencies. It needs an esbuild plugin to allow bundling to work correctly. Without it, we must build manually outside CDK first, and then hand over the build artifact to CDK. It would be much preferred to be able to use CDK esbuild bundling.

@github-actions
Copy link

This issue has received a significant amount of attention so we are automatically upgrading its priority. A member of the community will see the re-prioritization and provide an update on the issue.

@FelixRelli
Copy link

Any update on this? Due to a limitation of esbuild using import_meta.url evanw/esbuild#1492 in an ESM project does not generate valid code. One should not be forced to use something like https://github.com/mrgrain/cdk-esbuild or this workaround:

const dirName = import.meta.url
  ? dirname(fileURLToPath(import.meta.url))
  : __dirname;

@arliber
Copy link

arliber commented Jan 3, 2024

For anyone stumbling upon this thread while looking for a solution for AWS SAM - the following worked for me when I tried usage a package with .node file (ssh2):

    Metadata:
      BuildMethod: esbuild
      BuildProperties:
        Minify: true
        Target: node18
        Sourcemap: true
        Loader:
          - .node=text
        EntryPoints:
          - app.ts

Specifically the Loader property is what I needed

@jdrydn
Copy link

jdrydn commented Jan 3, 2024

Metadata:
  BuildProperties:
    Loader:
      - .node=text

Specifically the Loader property is what I needed

Depending on how you have CDK/SAM configured, bundling native dependencies like this is only fine if you've installed dependencies on the same OS/architecture as Lambda. If you've installed dependencies on Apple Silicon (darwin/amd64) don't expect this to work on Lambda (linux/amd64) out-of-the-box!

You could also build the dependencies with Docker before deploying:

$ docker run -it --rm -v $PWD:/workspace -w /workspace public.ecr.aws/sam/build-nodejs18.x npm install

@astuyve
Copy link

astuyve commented Mar 14, 2024

Hey friends!

Is there a timeline for this to be supported?

ESBuild recommends plugins as the solution for augmenting bundling with instrumentation or other build-time modifications, and the CDK's lack of compatibility is actively hampering customer adoption of CDK for users who require plugins. It seems that ESBuild has created a path forward.

Thanks!

@colifran colifran self-assigned this Mar 25, 2024
@colifran
Copy link
Contributor

colifran commented Mar 29, 2024

Hi everyone. As this issue has received a lot of attention, we wanted to look into what options are available to try to support this functionality.

Below I've detailed a high-level overview of the NodejsFunction and the current bundling mechanism as a way to start from the same context. Following that I've detailed three options we considered while exploring supporting this. Each option is followed with advantages and disadvantages. We're interested in feedback and any discussion on these three options -- additional solutions, things we may be missing in the current options, etc.

NodejsFunction Construct and Current Bundling Mechanism

NodejsFunction is a construct that represents a nodejs Lambda function bundled using esbuild. This construct is a wrapper around the standard Function construct from the aws-lambda module. In its present state, the NodejsFunction construct provides its users with two mechanisms for bundling Lambda handlers and their dependencies:

  1. Local bundling using the esbuild commands via CLI
  2. Docker bundling using esbuild commands

To select Docker bundling over local bundling the forceDockerBundling flag must be set to true as part of the BundlingOptions in the NodejsFunctionProps interface. If forceDockerBundling is false , then local bundling will be used.

The Function construct has three required properties: code , runtime , and handler . As a result of extending the Function construct, the NodejsFunction must pass these three parameters to the Function construct via a super call in its constructor. Notably, to satisfy the code property, the NodejsFunction uses a Bundling construct. The Bundling construct implements the BundlingOptions interface which defines properties used for local and Docker bundling. The Bundling construct exposes a static bundle method which returns AssetCode and encapsulates the logic used for bundling.

super(scope, id, {
...props,
runtime,
code: Bundling.bundle(scope, {
...props.bundling ?? {},
entry,
runtime,
architecture,
depsLockFilePath,
projectRoot,
}),
handler: handler.indexOf('.') !== -1 ? `${handler}` : `index.${handler}`,
});

public static bundle(scope: IConstruct, options: BundlingProps): AssetCode {
return Code.fromAsset(options.projectRoot, {
assetHash: options.assetHash,
assetHashType: options.assetHash ? cdk.AssetHashType.CUSTOM : cdk.AssetHashType.OUTPUT,
bundling: new Bundling(scope, options),
});
}

At a high-level, the logic contained in the Bundling construct follows a single, non-branching path through the constructor. The code through the constructor sets various Docker related class attributes defined on the BundlingOptions interface such as workingDirectory , entrypoint , volumes , command , etc. Notably, the image attribute is always set, but will be a “dummy” value if the Bundling construct is not constructed with forceDockerBundling or if esbuild package installation is not detected.

// Docker bundling
const shouldBuildImage = props.forceDockerBundling || !Bundling.esbuildInstallation;
this.image = shouldBuildImage ? props.dockerImage ?? cdk.DockerImage.fromBuild(path.join(__dirname, '..', 'lib'),
{
buildArgs: {
...props.buildArgs ?? {},
// If runtime isn't passed use regional default, lowest common denominator is node18
IMAGE: props.runtime.bundlingImage.image,
ESBUILD_VERSION: props.esbuildVersion ?? ESBUILD_MAJOR_VERSION,
},
platform: props.architecture.dockerPlatform,
})
: cdk.DockerImage.fromRegistry('dummy'); // Do not build if we don't need to
const bundlingCommand = this.createBundlingCommand({
inputDir: cdk.AssetStaging.BUNDLING_INPUT_DIR,
outputDir: cdk.AssetStaging.BUNDLING_OUTPUT_DIR,
esbuildRunner: 'esbuild', // esbuild is installed globally in the docker image
tscRunner: 'tsc', // tsc is installed globally in the docker image
osPlatform: 'linux', // linux docker image
});
this.command = props.command ?? ['bash', '-c', bundlingCommand];
this.environment = props.environment;
// Bundling sets the working directory to cdk.AssetStaging.BUNDLING_INPUT_DIR
// and we want to force npx to use the globally installed esbuild.
this.workingDirectory = props.workingDirectory ?? '/';
this.entrypoint = props.entrypoint;
this.volumes = props.volumes;
this.volumesFrom = props.volumesFrom;
this.user = props.user;
this.securityOpt = props.securityOpt;
this.network = props.network;
this.bundlingFileAccess = props.bundlingFileAccess;

The last part of the constructor sets the local attribute if forceDockerBundling is false . The local attribute is optional, but it serves as a signal that local bundling should be used.

// Local bundling
if (!props.forceDockerBundling) { // only if Docker is not forced
this.local = this.getLocalBundlingProvider();
}

The getLocalBundlingProvider method returns an implementation of the ILocalBundling interface which requires implementing the tryBundle method. The implementation given to local will result in local esbuild bundling via the CLI.

return {
tryBundle(outputDir: string) {
if (!Bundling.esbuildInstallation) {
process.stderr.write('esbuild cannot run locally. Switching to Docker bundling.\n');
return false;
}
if (!Bundling.esbuildInstallation.version.startsWith(`${ESBUILD_MAJOR_VERSION}.`)) {
throw new Error(`Expected esbuild version ${ESBUILD_MAJOR_VERSION}.x but got ${Bundling.esbuildInstallation.version}`);
}
const localCommand = createLocalCommand(outputDir, Bundling.esbuildInstallation, Bundling.tscInstallation);
exec(
osPlatform === 'win32' ? 'cmd' : 'bash',
[
osPlatform === 'win32' ? '/c' : '-c',
localCommand,
],
{
env: { ...process.env, ...environment },
stdio: [ // show output
'ignore', // ignore stdio
process.stderr, // redirect stdout to stderr
'inherit', // inherit stderr
],
cwd,
windowsVerbatimArguments: osPlatform === 'win32',
});
return true;
},
};

The code property expected by the Function construct must be an implementation of the abstract class Code . All implementations of Code have an implementation of bind which binds the Code construct to the Function construct. When bind is called for AssetCode a new S3 Asset will be created:

this.asset = new s3_assets.Asset(scope, 'Code', {
path: this.path,
deployTime: true,
...this.options,
});

When an Asset is created, a new AssetStage will also be created:

const staging = new cdk.AssetStaging(this, 'Stage', {
...props,
sourcePath: path.resolve(props.path),
follow: props.followSymlinks ?? toSymlinkFollow(props.follow),
assetHash: props.assetHash ?? props.sourceHash,
});

Finally, in the constructor of AssetStage the bundling logic is executed via the private bundling method. This method first looks for the local property defined in BundlingOptions and possibly set in the Bundling construct. If found, the tryBundle method is executed. If the result of tryBundle is false or if local is undefined, then Docker bundling is attempted:

private bundle(options: BundlingOptions, bundleDir: string) {
if (fs.existsSync(bundleDir)) { return; }
fs.ensureDirSync(bundleDir);
// Chmod the bundleDir to full access.
fs.chmodSync(bundleDir, 0o777);
let localBundling: boolean | undefined;
try {
process.stderr.write(`Bundling asset ${this.node.path}...\n`);
localBundling = options.local?.tryBundle(bundleDir, options);
if (!localBundling) {
const assetStagingOptions = {
sourcePath: this.sourcePath,
bundleDir,
...options,
};
switch (options.bundlingFileAccess) {
case BundlingFileAccess.VOLUME_COPY:
new AssetBundlingVolumeCopy(assetStagingOptions).run();
break;
case BundlingFileAccess.BIND_MOUNT:
default:
new AssetBundlingBindMount(assetStagingOptions).run();
break;
}
}

Option 1

This solution was considered as a way to maintain the current public API by only updating the underlying implementation details of existing constructs.

In this solution, bundling would still be configurable via the NodejsFunctionProps interface and new bundling options could be added to the bundling property as part of the BundlingOptions interface:

export interface BundlingOptions extends DockerRunOptions {
  /**
   * Whether to minify files when bundling.
   *
   * @default false
   */
  readonly minify?: boolean;
  /**
   * Whether to include source maps when bundling.
   *
   * @default false
   */
  readonly sourceMap?: boolean;
  /**
   * Source map mode to be used when bundling.
   * @see https://esbuild.github.io/api/#sourcemap
   *
   * @default SourceMapMode.DEFAULT
   */
  readonly sourceMapMode?: SourceMapMode;
  /**
   * Whether to include original source code in source maps when bundling.
   *
   * @see https://esbuild.github.io/api/#sources-content
   *
   * @default true
   */
  readonly sourcesContent?: boolean;
  /**
   * Target environment for the generated JavaScript code.
   *
   * @see https://esbuild.github.io/api/#target
   *
   * @default - the node version of the runtime
   */
  readonly target?: string;
  
  // ...
  
  readonly plugins?: [] EsbuildPlugins; <--- New interface we would need to define  
  
  // ...
}

Just like in the existing implementation, the BundlingOptions could be passed to the static bundle method on the Bundling construct to be used in configuring the bundling options for esbuild.

For local bundling, the implementation is contained within tryBundle . Consequently, this means that we could, in theory, update the existing implementation of tryBundle to use the esbuild API rather than the current exec call to execute an esbuild command via the CLI. This would follow the code path detailed in the overview above and would eventually be executed in the bundle method in AssetStaging:

private getLocalBundlingProvider(): cdk.ILocalBundling {
    // configuration / setup in outer scope

    return {
      async tryBundle(outputDir: string) {
        // configuration / setup in inner scope
        
        // error handling / check for valid local bundling
        
        await esbuild.build({
            // bundling options
        });

        return true;
      },
    };
  }
}

With this we encounter a problem - the CDK is built around synchronous operations. The idea here is that the output of synth should be deterministic. Esbuild offers a buildSync API, but per the esbuild API documentation plugins can only be used with the asynchronous API:

image

Additionally, Docker bundling must be considered and this appears to be another blocker. Specifically, how would the esbuild API be utilized within Docker. An obvious answer is using a build script, but the next question that follows is how would things like plugins be passed to the build script?

Advantages

  1. The public API for NodejsFunction remains the same and no code updates would be needed. This would provide a familiar experience with the benefits of the esbuild API under-the-hood.
  2. No new constructs means that implementing this can be accomplished faster and we wouldn’t have additional code to maintain.
  3. The existing tests would serve as a means to help guide the implementation since any failing tests are a signal of incorrect implementation.

Disadvantages

  1. Updating the implementation details of an existing construct could unknowingly introduce a breaking change. We also need to ensure that all existing bundling options are still being utilized by the API.
  2. This would eliminate the current bundling mechanism which eliminates the ability to bundle with the API vs. the CLI
  3. To offer support for plugins we would need to utilize the asynchronous API. Since the CDK is built on synchronous operations this wouldn't be practical.
  4. Docker bundling with the esbuild API would be a challenge. Specifically, how could information like plugins be passed into something like a build script to then be run in Docker?

Option 2

This solution was considered in parallel with first solution. The thought here was to create a new NodejsFunction (something like NodejsFunctionV2) construct that cleanly separates itself from the current construct. This new version would be a more "modern" NodejsFunction that utilizes the esbuild API for its bundling mechanism. Doing this would maintain the functionality of the existing construct while offering functionality provided via the esbuild API for those that want more control over the bundling configuration in the newer version.

For configuring the construct and esbuild, we could introduce a new interface named NodejsFunctionPropsV2 . Additionally, we could introduce another new interface named BundlingOptionsV2 which would allow us to add esbuild API specific configuration properties without introducing unusable properties on BundlingOptions.

export interface BundlingOptionsV2 {
    //...
    
    readonly plugins: EsbuildPlugin[]; <--- EsbuildPlugin would need to be created
    
    //...
}
export interface NodejsFunctionPropsV2 extends lambda.FunctionProps {
    // ...
    
    readonly bundling?: BundlingOptionsV2;
    
    // ...
}

From here, the implementation would be similar to the existing construct. The properties defined in NodejsFunctionPropsV2 would be passed to NodejsFunctionV2 for use during construction. We could create a BundlingV2 construct to use the esbuild API as its mechanism for bundling which would expose a static bundle method to return an AssetCode instance.

export class NodejsFunctionV2 extends lambda.Function {
  constructor(scope: Construct, id: string, props: NodejsFunctionPropsV2 = {}) {
    //...

    super(scope, id, {
      ...props,
      runtime,
      code: BundlingV2.bundle(scope, {
        ...props.bundling ?? {},
        entry,
        runtime,
        architecture,
        depsLockFilePath,
        projectRoot,
      }),
      handler: handler.indexOf('.') !== -1 ? `${handler}` : `index.${handler}`,
    });

    //...
  }
}
export class BundlingV2 implements cdk.BundlingOptions {
  public static bundle(scope: IConstruct, options: BundlingProps): AssetCode {
    return Code.fromAsset(options.projectRoot, {
      assetHash: options.assetHash,
      assetHashType: options.assetHash ? cdk.AssetHashType.CUSTOM : cdk.AssetHashType.OUTPUT,
      bundling: new BundlingV2(scope, options),
    });
  }
  
  //...
}

Unfortunately, from here the same two problems detailed in option 1 arise:

  1. Local bundling would need to use the asynchronous esbuild API to support plugins
  2. Bundling via Docker using the esbuild API would be challenging

Advantages

  1. Creating a new construct gives the ability to start fresh with our implementation of bundling using the esbuild API.
  2. We don’t need to concern ourselves with any potential breaking changes since we won’t be changing any existing behavior.
  3. We maintain the existing bundling mechanism which would provide the option of CLI or API for esbuild bundling.

Disadvantages

  1. A new NodejsFunctionV2 construct would be similar to the existing NodejsFunction which would introduce a lot of duplicate code.
  2. To offer support for plugins we would need to utilize the asynchronous API. Since the CDK is built on synchronous operations this wouldn't be practical.
  3. Docker bundling with the esbuild API would be a challenge. Specifically, how could information like plugins be passed into something like a build script to then be run in Docker?

Option 3

This solution takes the approach that, ultimately, the CDK wasn't developed as a build tool. With that, another approach is a solution that meets in the middle. Specifically, we could create a new Code static method called something along the lines of Code.fromBuiltAsset which would satisfy the code property defined on the FunctionProps interface.

new Function(this, 'LambdaFunction', {
    //...
    code: Code.fromBuiltAsset({
        command: ['node', './build-script.js'],
    }),
	//...
});

Additionally, we could expose the code property on the NodejsFunctionProps interface to allow Code.fromBuiltAsset to be utilized within the NodejsFunction as well. Right now it defaults to using Bundler.bundle.

This approach would subvert the control of bundling back to the user without requiring them to first bundle and then supply the bundled code. Instead, at build time, the CDK would execute the build script on the users machine and look for the bundled code in a defined outfile location. The user will have everything installed on their machine that is necessary for bundling and they are able to set-up their build script in whatever way they choose.

This solution would provide convenience in that we still are bundling their code at build time, but the user has the freedom to choose the bundler, the API, the configuration, etc.

Advantages

  1. The user could choose any bundler they want as long as everything needed is installed on their machine.
  2. The user could configure the bundler API they're using in whatever way meets their needs.
  3. The build script would be executed when cdk synth is run. The user doesn't need to bundle first, then supply the output file, and then run cdk synth. Ideally this keeps development streamlined.
  4. Users that aren't concerned with having more control over bundling can still use the existing functionality offered by NodejsFunction.

Disadvantages

  1. To benefit from this, users would need to write their own build script.
  2. Bundling would no longer be hidden as an implementation detail with this option.

Recommendation

We see option 3 as being the best solution to move forward with. This approach would meet in the middle by providing a solution that will bundle handler code and dependencies when running cdk synth, while giving the user control of the bundling mechanism and configuration used.

@astuyve
Copy link

astuyve commented Mar 29, 2024

Thanks for this clear and well-reasoned analysis @colifran!

Although writing a custom build script does seem heavy to support plugins, I recognize that the synchronous nature of the CDK and the asynchronous esbuild plugin design represents a bit of an issue. Is your concern primarily technical/performance based? Or just generally that the CDK team doesn't want to support asynchronous packaging/synth to prevent someone from say, calling endpoints and introducing further nondeterminism?

Depending on that answer, option 1 could be okay. Option 3 seems good overall but certainly represents more work for users who will have to roll their own build scripts. Not a blocker certainly, but just an observation. I'm not sure versioning NodeJSFunction is ideal, but I have less strong opinions here.

Overall I'd be pleased with either choice. Thanks again!

@colifran
Copy link
Contributor

colifran commented Apr 1, 2024

@astuyve Thanks for your feedback! The concern right now is technical based. The current CDK architecture makes a lot of core assumptions based on operations taking place synchronously. There is a long discussion here where one of the creators of the CDK shares their opinion on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-lambda-nodejs effort/large Large work item – several weeks of effort feature-request A feature should be added or improved. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.