fix(lambda-nodejs): unable to use `nodeModules` with pnpm #21911

cecheta · 2022-09-03T21:39:08Z

Fixes #21910

By default, pnpm uses symlinks when installing dependencies, and modules only have access to dependencies in their package.json. This means when trying to bundle, dependencies of depedencies are not installed. This PR fixes this by using the --config.node_linker=hoisted flag to create a flat node_modules structure. Docs.

The second problem this PR fixes is when using pnpm workspaces. With workspaces, modules are installed within the workspace the command is run in. This means that when installing as part of bundling in a nested directory, no node_modules directory is produced at all. By creating a new pnpm-workspace.yaml file locally before installing, this essentially creates a new (nested) workspace, and node_modules is installed here. An empty file is enough for this to suffice.

The PR also removes a .modules.yaml file from the node_modules after installing. This file contains a datetime of the last install, therefore it would cause the lambda code to change on each deployment if it was included in the bundle.

I have tested this fix locally on both Mac and Windows. I have also included an integration test which failed before these changes, however I am not sure if it should be included due to the node_modules in the assets.

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

gitpod-io · 2022-09-03T21:39:15Z

TheRealAmazonKendra

Thank you for your contribution! I don't think we can review this effectively at present. With 395 changes files, this almost certainly contains some unintended changes. I can't tell now many of these would be intentionally added to this PR.

Pull request has been modified.

cecheta · 2022-09-12T16:53:32Z

Thank you for your contribution! I don't think we can review this effectively at present. With 395 changes files, this almost certainly contains some unintended changes. I can't tell now many of these would be intentionally added to this PR.

The large number of files comes from the assets in the integration tests, as I tried to write a test for a scenario which wouldn't have worked prior to this fix.

I've removed the integ test for now

TheRealAmazonKendra · 2022-10-19T02:39:11Z

Thank you for your contribution! I don't think we can review this effectively at present. With 395 changes files, this almost certainly contains some unintended changes. I can't tell now many of these would be intentionally added to this PR.

The large number of files comes from the assets in the integration tests, as I tried to write a test for a scenario which wouldn't have worked prior to this fix.

I've removed the integ test for now

In that case, I'll want that test back in once we've done a review of this. Apologies for assuming it was an unintentional addition.

TheRealAmazonKendra · 2022-10-19T02:41:08Z

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

@@ -298,13 +299,17 @@ interface BundlingCommandOptions {
 class OsCommand {
  constructor(private readonly osPlatform: NodeJS.Platform) {}

-  public writeJson(filePath: string, data: any): string {
-    const stringifiedData = JSON.stringify(data);
+  public write(filePath: string, data: any): string {


Can you provide further context on why this additional function is needed?

I have created write() which takes a string and outputs it to a file. This is required for the pnpm-workspace.yaml file.

writeJson() receives any input, stringifies it, then calls write().

This way I have avoided duplicate code.

TheRealAmazonKendra · 2022-10-19T02:41:35Z

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

+    return `echo '${data}' > "${filePath}"`;
+  }
+
+  public writeJson(filePath: string, data: any): string {


And why this doesn't need the extra check for OS?

The OS check has been moved to the write() function

TheRealAmazonKendra · 2022-10-19T02:44:03Z

packages/@aws-cdk/aws-lambda-nodejs/lib/package-manager.ts

@@ -39,7 +39,7 @@ export class PackageManager {
      case LockFile.PNPM:
        return new PackageManager({
          lockFile: LockFile.PNPM,
-          installCommand: logLevel && logLevel !== LogLevel.INFO ? ['pnpm', 'install', '--reporter', 'silent'] : ['pnpm', 'install'],
+          installCommand: logLevel && logLevel !== LogLevel.INFO ? ['pnpm', 'install', '--reporter', 'silent', '--shamefully-hoist'] : ['pnpm', 'install', '--shamefully-hoist'],


Pretend I don't know shit about pnpm here (because I don't). What is --shamefully-hoist doing here?

I'm now using --config.node_linker=hoisted, as explained in the PR description

TheRealAmazonKendra · 2022-10-19T02:48:10Z

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

@@ -219,6 +219,7 @@ export class Bundling implements cdk.BundlingOptions {

      // Create dummy package.json, copy lock file if any and then install
      depsCommand = chain([
+        this.packageManager.lockFile === LockFile.PNPM ? osCommand.write(pathJoin(options.outputDir, 'pnpm-workspace.yaml'), ''): '',


If the only instance where you're using write has no value for data, I'm not sure I get why that function takes in two values and why it's being used here. Are you just creating an empty file titled pnpm-workspace.yaml? Also, why use an empty string instead of undefined for this.packageManager.lockFile if it's not pnpm?

Yes creating an empty pnpm-workspace.yaml file avoids a problem when using workspaces. I have updated the PR description to explain.

I have created a general write() function so it could be used by someone else in the future if they need to write a (non-empty) file, rather than creating a writeEmptyFile() function which seems a bit bespoke.

I am using an empty string as chain() takes an array of strings. It will be removed then all the commands are joined together into a single command.

Pull request has been modified.

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

cecheta · 2022-10-29T00:38:55Z

Thank you for your contribution! I don't think we can review this effectively at present. With 395 changes files, this almost certainly contains some unintended changes. I can't tell now many of these would be intentionally added to this PR.

The large number of files comes from the assets in the integration tests, as I tried to write a test for a scenario which wouldn't have worked prior to this fix.
I've removed the integ test for now

In that case, I'll want that test back in once we've done a review of this. Apologies for assuming it was an unintentional addition.

I have added the test back, again the large number of files is due to the node_modules included in the snapshot.

I have chosen axios for the test as this package has a dependency, therefore demonstrating the issue, however it only has one dependency to hopefully keep the asset size low

aws-cdk-automation · 2022-12-01T00:10:14Z

This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

corymhall

@cecheta this looks great! I just have a couple of minor comments.

corymhall · 2022-12-19T19:35:37Z

buildspec-pr.yaml

@@ -16,6 +16,9 @@ phases:
      # Install yarn if it wasn't already present in the image
      - yarn --version || npm -g install yarn

+      # Install pnpm if it wasn't already present in the image
+      - pnpm --version || npm -g install pnpm


We shouldn't need this.

I get bash: pnpm: command not found if I remove it, although I'm not sure if it's needed for just one of the files or both

corymhall · 2022-12-19T19:35:44Z

buildspec.yaml

@@ -16,6 +16,9 @@ phases:
      # Install yarn if it wasn't already present in the image
      - yarn --version || npm -g install yarn

+      # Install pnpm if it wasn't already present in the image
+      - pnpm --version || npm -g install pnpm


We shouldn't need this.

corymhall · 2022-12-19T19:37:19Z

packages/@aws-cdk/aws-lambda-nodejs/test/integ.dependencies-pnpm.ts

+});
+
+new integ.IntegTest(app, 'PnpmTest', {
+  testCases: [stack],


Suggested change

testCases: [stack],

testCases: [stack],

stackUpdateWorkflow: false, // this will tell the runner to not check in assets.

Done, thank you

FYI I ran into a bug when updating the tests, raised #23407

corymhall · 2022-12-19T19:41:33Z

yarn.lock

@@ -2668,6 +2668,13 @@ aws4@^1.8.0:
  resolved "https://registry.npmjs.org/aws4/-/aws4-1.11.0.tgz#d61f46d83b2519250e2784daf5b09479a8b41c59"
  integrity sha512-xh1Rl34h6Fi1DC2WWKfxUTVqRsNnr6LsKz2+hfwDxQJWmrx8+c7ylaqBMcHfl1U1r2dsifOvKX3LQuLNZ+XSvA==

+axios@0.26.1:


I don't think this file should have been updated.

Changed the version of axios to use a version already in the lock file

Pull request has been modified.

corymhall · 2022-12-22T14:40:37Z

packages/@aws-cdk/aws-lambda-nodejs/test/integ.dependencies-pnpm.ts

+    // Will be installed, not bundled
+    // (axios is a package with sub-dependencies,
+    // will be used to ensure pnpm bundling works as expected)
+    nodeModules: ['axios'],


Can you add forceDockerBundling: true? I think if we force it to bundle inside docker it removes the need to add any of the dependencies in the repo.

I switched to Docker bundling, but doing so demonstrated another issue, which is that when you use Docker bundling, the pnpm store is included in the asset by default. This is also the case on main.

I've added a tmp directory for the store into the Dockerfile, and added --config.package-import-method=clone-or-copy when running pnpm install which resolves this issue

Pull request has been modified.

corymhall

Just one minor thing and then this is ready to go! 🚀

corymhall · 2022-12-27T13:01:36Z

packages/@aws-cdk/aws-lambda-nodejs/package.json

    "@types/jest": "^27.5.2",
+    "axios": "^0.27.2",


Suggested change

"axios": "^0.27.2",

I don't think this is needed anymore

For some reason, the tests in test/docker.test.js are now failing

I can see in the logs:

time="2022-12-27T14:51:42.954645757Z" level=info msg="Attempting next endpoint for pull after error: denied: User: arn:aws:sts::464831875156:assumed-role/Ops-cdk-autobuild-prs-AutoBuildv2ProjectRoleC688F4-CY3ZTUL6CY8Q/AWSCodeBuild-b78b1055-51c8-4066-8449-8e563d561d94 is not authorized to perform: ecr:BatchGetImage on resource: arn:aws:ecr:us-east-1:464831875156:repository/library/esbuild because no identity-based policy allows the ecr:BatchGetImage action"

Squashing commits seemed to fix it, not sure why

Pull request has been modified.

mergify · 2022-12-28T19:42:23Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2022-12-28T20:20:50Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 199cdd2
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2022-12-28T20:21:06Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Fixes aws#21910 By default, pnpm uses symlinks when installing dependencies, and modules only have access to dependencies in their `package.json`. This means when trying to bundle, dependencies of depedencies are not installed. This PR fixes this by using the ` --config.node_linker=hoisted` flag to create a flat `node_modules` structure. [Docs](https://pnpm.io/npmrc#node-linker). The second problem this PR fixes is when using pnpm workspaces. With workspaces, modules are installed within the workspace the command is run in. This means that when installing as part of bundling in a nested directory, no `node_modules` directory is produced at all. By creating a new `pnpm-workspace.yaml` file locally before installing, this essentially creates a new (nested) workspace, and `node_modules` is installed here. An empty file is enough for this to suffice. The PR also removes a `.modules.yaml` file from the `node_modules` after installing. This file contains a datetime of the last install, therefore it would cause the lambda code to change on each deployment if it was included in the bundle. I have tested this fix locally on both Mac and Windows. I have also included an integration test which failed before these changes, however I am not sure if it should be included due to the `node_modules` in the assets. ---- ### All Submissions: * [X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [X] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [X] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

aws-cdk-automation requested a review from a team September 3, 2022 21:39

github-actions bot added bug This issue is a bug. p2 labels Sep 3, 2022

TheRealAmazonKendra previously requested changes Sep 9, 2022

View reviewed changes

TheRealAmazonKendra previously requested changes Oct 19, 2022

View reviewed changes

cecheta marked this pull request as draft October 28, 2022 19:24

aws-cdk-automation previously requested changes Oct 28, 2022

View reviewed changes

github-actions bot added the effort/medium Medium work item – several days of effort label Oct 29, 2022

cecheta marked this pull request as ready for review October 29, 2022 00:30

cecheta marked this pull request as draft October 29, 2022 17:32

cecheta marked this pull request as ready for review October 29, 2022 19:05

corymhall previously requested changes Dec 19, 2022

View reviewed changes

corymhall self-assigned this Dec 19, 2022

corymhall previously requested changes Dec 22, 2022

View reviewed changes

corymhall previously requested changes Dec 27, 2022

View reviewed changes

fix(lambda-nodejs): unable to use nodeModules with pnpm

63300f8

corymhall approved these changes Dec 28, 2022

View reviewed changes

Merge branch 'main' into pnpm

199cdd2

mergify bot merged commit 7c752db into aws:main Dec 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(lambda-nodejs): unable to use `nodeModules` with pnpm #21911

fix(lambda-nodejs): unable to use `nodeModules` with pnpm #21911

cecheta commented Sep 3, 2022 •

edited

Loading

gitpod-io bot commented Sep 3, 2022

TheRealAmazonKendra left a comment

cecheta commented Sep 12, 2022

TheRealAmazonKendra commented Oct 19, 2022

TheRealAmazonKendra Oct 19, 2022

cecheta Oct 29, 2022 •

edited

Loading

TheRealAmazonKendra Oct 19, 2022

cecheta Oct 29, 2022

TheRealAmazonKendra Oct 19, 2022

cecheta Oct 29, 2022

TheRealAmazonKendra Oct 19, 2022

cecheta Oct 29, 2022 •

edited

Loading

aws-cdk-automation left a comment

cecheta commented Oct 29, 2022 •

edited

Loading

aws-cdk-automation commented Dec 1, 2022

corymhall left a comment

corymhall Dec 19, 2022

cecheta Dec 20, 2022

corymhall Dec 19, 2022

corymhall Dec 19, 2022

cecheta Dec 20, 2022

cecheta Dec 20, 2022

corymhall Dec 19, 2022

cecheta Dec 20, 2022

corymhall Dec 22, 2022

cecheta Dec 22, 2022 •

edited

Loading

corymhall left a comment

corymhall Dec 27, 2022

cecheta Dec 27, 2022

cecheta Dec 27, 2022

cecheta Dec 28, 2022

mergify bot commented Dec 28, 2022

aws-cdk-automation commented Dec 28, 2022

mergify bot commented Dec 28, 2022

	testCases: [stack],
	testCases: [stack],
	stackUpdateWorkflow: false, // this will tell the runner to not check in assets.

fix(lambda-nodejs): unable to use nodeModules with pnpm #21911

fix(lambda-nodejs): unable to use nodeModules with pnpm #21911

Conversation

cecheta commented Sep 3, 2022 • edited Loading

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Sep 3, 2022

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

cecheta commented Sep 12, 2022

TheRealAmazonKendra commented Oct 19, 2022

Choose a reason for hiding this comment

cecheta Oct 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cecheta Oct 29, 2022 • edited Loading

Choose a reason for hiding this comment

aws-cdk-automation left a comment

Choose a reason for hiding this comment

cecheta commented Oct 29, 2022 • edited Loading

aws-cdk-automation commented Dec 1, 2022

corymhall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cecheta Dec 22, 2022 • edited Loading

Choose a reason for hiding this comment

corymhall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Dec 28, 2022

aws-cdk-automation commented Dec 28, 2022

AWS CodeBuild CI Report

mergify bot commented Dec 28, 2022

fix(lambda-nodejs): unable to use `nodeModules` with pnpm #21911

fix(lambda-nodejs): unable to use `nodeModules` with pnpm #21911

cecheta commented Sep 3, 2022 •

edited

Loading

cecheta Oct 29, 2022 •

edited

Loading

cecheta Oct 29, 2022 •

edited

Loading

cecheta commented Oct 29, 2022 •

edited

Loading

cecheta Dec 22, 2022 •

edited

Loading