(aws-lambda-nodejs): Wrong @aws-sdk bundling when using format: OutputFormat.ESM and externalModules: []

WtfJoke commented 6 months ago

Describe the bug

We have a project using esModules. We want to utilize esModules also for our Lambdas. When using NodeJsFunction and provide the following custom bundling options: bundling: { externalModules: [], format: OutputFormat.ESM }, the resulting .mjs file contains wrong imports (dist-cjs instead of dist-es).

Here is the section of the bundled .mjs file

2024-02-29T13:52:13.280Z    undefined   ERROR   Uncaught Exception  
{
    "errorType": "Error",
    "errorMessage": "Dynamic require of \"os\" is not supported",
    "stack": [
        "Error: Dynamic require of \"os\" is not supported",
        "    at file:///var/task/index.mjs:12:9",
        "    at node_modules/@smithy/shared-ini-file-loader/dist-cjs/getHomeDir.js (file:///var/task/index.mjs:1877:16)",
        "    at __require2 (file:///var/task/index.mjs:18:50)",
        "    at node_modules/@smithy/shared-ini-file-loader/dist-cjs/index.js (file:///var/task/index.mjs:1991:29)",
        "    at __require2 (file:///var/task/index.mjs:18:50)",
        "    at node_modules/@smithy/node-config-provider/dist-cjs/index.js (file:///var/task/index.mjs:2152:41)",
        "    at __require2 (file:///var/task/index.mjs:18:50)",
        "    at node_modules/@smithy/middleware-endpoint/dist-cjs/adaptors/getEndpointFromConfig.js (file:///var/task/index.mjs:2231:34)",
        "    at __require2 (file:///var/task/index.mjs:18:50)",
        "    at node_modules/@smithy/middleware-endpoint/dist-cjs/index.js (file:///var/task/index.mjs:2516:40)"
    ]
}

When setting only format option bundling: { format: OutputFormat.ESM },, everything works as expected, because the provided aws-sdk is loaded (instead of the bundled one).

We set externalModules: [] because that leads to lower cold start times (reference: https://github.com/aws/aws-cdk/issues/25492), which means the @aws-sdk is bundled.

Expected Behavior

I expect that no CommonJS will be used when setting OutputFormat.esm in the bundling options.

Current Behavior

The resulting .mjs file contains commonJs references, which leads to the function crashing:

// node_modules/@aws-sdk/util-user-agent-node/dist-cjs/index.js
var require_dist_cjs42 = __commonJS({
  "node_modules/@aws-sdk/util-user-agent-node/dist-cjs/index.js"(exports, module) {

The function crashes with the following exception:

2024-02-29T13:52:13.280Z    undefined   ERROR   Uncaught Exception  
{
    "errorType": "Error",
    "errorMessage": "Dynamic require of \"os\" is not supported",
    "stack": [
        "Error: Dynamic require of \"os\" is not supported",
        "    at file:///var/task/index.mjs:12:9",
        "    at node_modules/@smithy/shared-ini-file-loader/dist-cjs/getHomeDir.js (file:///var/task/index.mjs:1877:16)",
        "    at __require2 (file:///var/task/index.mjs:18:50)",
        "    at node_modules/@smithy/shared-ini-file-loader/dist-cjs/index.js (file:///var/task/index.mjs:1991:29)",
        "    at __require2 (file:///var/task/index.mjs:18:50)",
        "    at node_modules/@smithy/node-config-provider/dist-cjs/index.js (file:///var/task/index.mjs:2152:41)",
        "    at __require2 (file:///var/task/index.mjs:18:50)",
        "    at node_modules/@smithy/middleware-endpoint/dist-cjs/adaptors/getEndpointFromConfig.js (file:///var/task/index.mjs:2231:34)",
        "    at __require2 (file:///var/task/index.mjs:18:50)",
        "    at node_modules/@smithy/middleware-endpoint/dist-cjs/index.js (file:///var/task/index.mjs:2516:40)"
    ]
}

Reproduction Steps

Run cdk synth on that project: https://github.com/WtfJoke/cdk-nodejs-esm-bug-reproducer
Inspect the resulting .mjs file

Alternatively:

Deploy that project: https://github.com/WtfJoke/cdk-nodejs-esm-bug-reproducer
Run the CdkWorkshopStack-MyLambda lambda and inspect the error

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

10.2.4

Framework Version

No response

Node.js Version

v20.11.0

OS

windows (wsl)

Language

TypeScript

Language Version

5.3.3

Other information

No response

pahud commented 6 months ago

This would need further investigation. I'm making it a p1 but we welcome any pull requests from the community.

Tietew commented 6 months ago

Following settings works for me: For details, see https://esbuild.github.io/api/#main-fields

const banner =
  "const require = (await import('node:module')).createRequire(import.meta.url);const __filename = (await import('node:url')).fileURLToPath(import.meta.url);const __dirname = (await import('node:path')).dirname(__filename);";
const externalModules = [
  '@aws-sdk/credential-provider-cognito-identity',
  '@aws-sdk/credential-provider-http',
  '@aws-sdk/credential-provider-ini',
  '@aws-sdk/credential-provider-process',
  '@aws-sdk/credential-provider-sso',
  '@aws-sdk/credential-provider-web-identity',
  '@smithy/credential-provider-imds',
];

new nodejs.NodejsFunction(this, 'Function', {
  entry: 'function.ts',
  runtime: lambda.Runtime.NODEJS_20_X,
  bundling: {
    banner,
    externalModules,
    format: nodejs.OutputFormat.ESM,
    mainFields: ['module', 'main'],
  },
});

The externalModules above prevents to bundle unneeded credential providers on Lambda.

WtfJoke commented 6 months ago

Specifying mainFields: ['module', 'main'] works.

The cdk docs to mainFields outlines that too:

How to determine the entry point for modules. Try ['module', 'main'] to default to ES module versions.

It seems it would make sense to specify mainFields to that value when you use OutputFormat.ESM.

It seems also an issue with aws-sdk bundling as well, according to https://github.com/evanw/esbuild/issues/2692 and https://github.com/aws/aws-sdk-js-v3/issues/4217. If I understand the issues correctly, when these would be fixed its not needed to overwrite mainFields.

Vandita2020 commented 4 months ago

Hey @WtfJoke, this is a dependency issue. ESBuild converts import statements in bundled packages to require but not vice versa. So when you emit to esm you'll get this error if any of your code is commonjs.

There are workarounds for this,

As mentioned by @Tietew using mainFields: ['module', 'main'] where we tell esbuild to use the module entry point.
Another one is to set banner to inject require into the script using banner: "import { createRequire } from 'module';const require = createRequire(import.meta.url)

WtfJoke commented 4 months ago

Hey @Vandita2020

Yeah I'm currently using the mentioned workarounds (mainfields and banner), but I would have expected that this workarounds shouldn't be necessary when using only esModules compatible libraries like in this case.

Have you seen my reproducer here: https://github.com/WtfJoke/cdk-nodejs-esm-bug-reproducer? The code in question:

import { SSM } from "@aws-sdk/client-ssm";

export const handler = async () => {
  new SSM().putParameter({
    Name: "my-param",
    Value: "my-value",
    Overwrite: true,
  });
};

It only uses @aws-sdk v3 as dependency, which is also ESM. So for me it looks like either issue with aws-sdk (not correct way of distributing esmodules code) or aws-cdk (not correct esbuild bundling settings).

Do you think that this issue should be closed in favour of https://github.com/aws/aws-sdk-js-v3/issues/4217 or a new issue in aws-sdk?

Vandita2020 commented 4 months ago

Hey @WtfJoke, I have replicated your code and worked with it. I also made modifications to simplify it by bundling the code without using CDK. Additionally, I have discussed this matter with AWS-SDK team, and it appears that everything points this to be an issue with ESBuild.

WtfJoke commented 4 months ago

Hey @Vandita2020

Thanks for taking the time, bundle the code without using CDK and discussing this also with the AWS-SDK team. Appreciate that.

Currently I do not understand why your mentioned issue is the cause for that (as far as I see, there is also no response from the esbuild or the aws sdk maintainers there). Is it possible to get a bit more of an explanation here?

There exists a previous issue in esbuild for the aws sdk, where the esbuild maintainer responded, concluding that there is an issue in the aws sdk.

For me it looks a bit like a deadlock here 🙈

Any ideas how we can progress from here?

P.S. For cdk/NodeJsFunction a solution could be to default the mainFields to ['module', 'main'] so ESM is forced when specifying OutputFormat.ESM. I would be open for creating this PR, if thats a reasonable solution.

scanlonp commented 3 months ago

Hey @WtfJoke, I am glad there is a workaround for this issue, but I am hesitant to make it the default behavior.

Setting this default value for mainFields when OutputFormat.ESM is provided could introduce unexpected or breaking behavior. When suggesting this workaround, the esbuild maintainer notes that it is not default in esbuild since it could break certain package configurations.

I am most comfortable with a user setting mainFields manually when they run into this issue. This seems better to be used as a workaround rather than incorporate it into our logic. Reading through the issues opened in esbuild and the SDK, it does appear to be an SDK issue. However, it does not appear that there will be a fix there in the short term since the SDK issue is labeled p3 at the time being.

I am concerned that this issue comes from such a basic configuration: do not bundle the SDK and use ESM. If necessary, we could narrow a fix in the CDK to only this configuration. Say checking if external modules is empty and the output format is ESM, then setting mainFields.

That would add logic here:

// the SDK does not handle ESM bundling correctly
// to workaround, set mainFields
const bundledSdkMainFields = [];
if (externals.length && this.props.format === OutputFormat.ESM) {
  bundledSdkMainFields = ['module', 'main'];
}

and this line becomes:

...this.props.mainFields ? [`--main-fields=${this.props.mainFields.join(',')}`] : bundledSdkMainFields,

I will check with the team, but I would prefer the workaround unless this issue is run into by many users. In any case, I will keep this open so that people can find the workaround more easily.

kuhe commented 2 months ago

try using https://github.com/aws/aws-sdk-js-v3/releases/tag/v3.592.0 or later of the AWS SDK.

We (AWS SDK) recently added conditional exports (package.json exports) in two of the SDK's core dependencies, and it needed a patch for bundlers in Node.js specifically, to add the non-spec "module" field recognizable by esbuild.

WtfJoke commented 2 months ago

@kuhe I've update to the latest available version 3.600.0 (see reproducer), but sadly this doesnt fix the issue

@scanlonp First of thanks for your detailed answer/analysis.

it does appear to be an SDK issue. However, it does not appear that there will be a fix there in the short term since the SDK issue is labeled p3 at the time being.

Looking forward to have it fixed there.

a basic configuration: do not bundle the SDK and use ESM

Just a comment to that: IMHO this should be the default behaviour going forward. As bundling the sdk on your own reduces cold starts (see https://github.com/aws/aws-cdk/issues/25492 (on a sidenote: this issue should still be open) and ESM has better three shaking support and is considered the future module system. If I remember correctly, the AWS docs advised on bundling the sdk on your own and dont rely on the supplied one (however I cant find the docs atm).

I would also be interested on how people use this OutputFormat.ESM setting. I could imagine that everyone who uses this setting is forced to also set mainfields this way :D

Say checking if external modules is empty and the output format is ESM, then setting mainFields.

Appreciate your openness about a solution 😄 However I'm hesitant to say that we should follow this route.

aws / aws-cdk