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

lambda-nodejs: Bundling fails pnpm version >= 8.4.0 #26478

Closed
damienhill opened this issue Jul 24, 2023 · 2 comments · Fixed by #26479
Closed

lambda-nodejs: Bundling fails pnpm version >= 8.4.0 #26478

damienhill opened this issue Jul 24, 2023 · 2 comments · Fixed by #26479
Labels
@aws-cdk/aws-lambda-nodejs bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.

Comments

@damienhill
Copy link
Contributor

damienhill commented Jul 24, 2023

Describe the bug

Previous issue #25612.

When bundling a lambda function using lambda_nodejs with no dependencies and with pnpm version 8.4.0 or higher the bundling process fails as the rm command doesn't correctly handle missing file.

My recent pull request to fix this was merged into release 2.88.0 but unfortunately failed to resolve issue as the -f flag was appended after the path which still throws an error when file doesn't exist.

Expected Behavior

The bundling succeeds (no need to delete this file).

Current Behavior

Bundling fails with error:

Bundling asset stack/lambda-fn/Code/Stage...

  ...dk.out/bundling-temp-d1f50916a16a3a6ba493bb8442e9fc9680e83df6b64d7a5183903277f0923fc7/index.js  2.1kb

⚡ Done in 8ms
Already up to date
Done in 314ms
rm: [..]/cdk.out/bundling-temp-d1f50916a16a3a6ba493bb8442e9fc9680e83df6b64d7a5183903277f0923fc7/node_modules/.modules.yaml: No such file or directory

...

Error: Failed to bundle asset stack/lambda-fn/Code/Stage, bundle output is located at [...] exited with status 1

Reproduction Steps

Simple lambda construct :

new lambda.NodejsFunction(this, 'MyFunction', {
  runtime: lambda.Runtime.NODEJS_16_X,
  entry: path.join(__dirname, 'lambda', 'src/index.ts'),
});

/lambda/package.json

{
  "dependencies": {
    "aws-lambda": "^1.0.7"
  },
}

/lambda/src/index.ts

import { DefineAuthChallengeTriggerHandler } from 'aws-lambda';

export const handler: DefineAuthChallengeTriggerHandler = async (event) => {
  return event;
};

/package.json

{
  "dependencies": {
    "aws-cdk": "^2.70.0",
    "aws-cdk-lib": "2.67.0",
    "constructs": "^10.0.0",
  },
  "devDependencies": {
    "esbuild": "^0.17.11",
    "ts-node": "^10.0.0",
    "typescript": "4.9.5"
  }
}

pnpm version:

  • v8.4.0 -> bundling fails
  • below v8.4.0 -> bundling succeeds

Possible Solution

In the bundling.ts source file the -f flag should come before the path in the rm command so the command doesn't error on file not existing.

aws-cdk/packages/aws-cdk-lib/aws-lambda-nodejs/lib/bundling.ts

isPnpm ? osCommand.remove(pathJoin(options.outputDir, 'node_modules', '.modules.yaml')) + ' -f' : '', // Remove '.modules.yaml' file which changes on each deployment

Additional Information/Context

The pnpm v8.0.4 release note specifies clearly :

Do not create a node_modules folder with a .modules.yaml file if there are no dependencies inside node_modules.
Since esbuild bundles the dependencies (unless specified otherwise through lambda_nodejs.NodejsFunctionProps['bundling]['nodeModules']), this will break synthesis of most lambda functions.

CDK CLI Version

2.88.0 (build 5d497f9)

Framework Version

No response

Node.js Version

v16.19.1

OS

macOs 13.3.1 (22E261)

Language

Typescript

Language Version

TypeScript (5.0.4)

Other information

No response

@damienhill damienhill added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jul 24, 2023
damienhill added a commit to damienhill/aws-cdk that referenced this issue Jul 24, 2023
Fixed location of -f flag in rm command for pnpm esbuild bundling step when removing node_modules/.modules.yaml from output dir.

Signed-off-by: Damien Hill <[email protected]>
@cchanche
Copy link

Thank you for handling this

@mergify mergify bot closed this as completed in #26479 Jul 24, 2023
mergify bot pushed a commit that referenced this issue Jul 24, 2023
Fix issue with order of `-f` flag and file path in `rm` command for `pnpm` esbuild bundling step to remove `node_modules/.modules.yaml` from output dir. This is continuing to cause bundling step to fail for `pnpm` >= 8.4.0 with no external `node_modules` specified per issue #26478.

Solved by moving the `-f` flag before file path in the `rm` command and updating relevant unit test. Please note that I haven't adjusted the `del` command for windows env as not sure if same issue occurs in that env.

Exemption Request: No changes to integration test output of `aws-lambda-nodejs/test/integ.dependencies-pnpm.js` and don't feel this warrants a separate integration test.

Closes #26478.

----

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

⚠️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.

bmoffatt pushed a commit to bmoffatt/aws-cdk that referenced this issue Jul 29, 2023
Fix issue with order of `-f` flag and file path in `rm` command for `pnpm` esbuild bundling step to remove `node_modules/.modules.yaml` from output dir. This is continuing to cause bundling step to fail for `pnpm` >= 8.4.0 with no external `node_modules` specified per issue aws#26478.

Solved by moving the `-f` flag before file path in the `rm` command and updating relevant unit test. Please note that I haven't adjusted the `del` command for windows env as not sure if same issue occurs in that env.

Exemption Request: No changes to integration test output of `aws-lambda-nodejs/test/integ.dependencies-pnpm.js` and don't feel this warrants a separate integration test.

Closes aws#26478.

----

*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
@aws-cdk/aws-lambda-nodejs bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants