Skip to content

Commit

Permalink
fix(lambda-nodejs): unable to use nodeModules with pnpm (#21911)
Browse files Browse the repository at this point in the history
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](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*
  • Loading branch information
cecheta authored Dec 28, 2022
1 parent b195465 commit 7c752db
Show file tree
Hide file tree
Showing 20 changed files with 921 additions and 13 deletions.
5 changes: 5 additions & 0 deletions packages/@aws-cdk/aws-lambda-nodejs/lib/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,11 @@ RUN mkdir /tmp/yarn-cache && \
chmod -R 777 /tmp/yarn-cache && \
yarn config set cache-folder /tmp/yarn-cache

# Ensure all users can write to pnpm cache
RUN mkdir /tmp/pnpm-cache && \
chmod -R 777 /tmp/pnpm-cache && \
pnpm config --global set store-dir /tmp/pnpm-cache

# Disable npm update notifications
RUN npm config --global set update-notifier false

Expand Down
29 changes: 24 additions & 5 deletions packages/@aws-cdk/aws-lambda-nodejs/lib/bundling.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import * as path from 'path';
import { Architecture, AssetCode, Code, Runtime } from '@aws-cdk/aws-lambda';
import * as cdk from '@aws-cdk/core';
import { PackageInstallation } from './package-installation';
import { PackageManager } from './package-manager';
import { LockFile, PackageManager } from './package-manager';
import { BundlingOptions, OutputFormat, SourceMapMode } from './types';
import { exec, extractDependencies, findUp, getTsconfigCompilerOptions } from './util';

Expand Down Expand Up @@ -229,12 +229,16 @@ export class Bundling implements cdk.BundlingOptions {

const lockFilePath = pathJoin(options.inputDir, this.relativeDepsLockFilePath ?? this.packageManager.lockFile);

const isPnpm = this.packageManager.lockFile === LockFile.PNPM;

// Create dummy package.json, copy lock file if any and then install
depsCommand = chain([
isPnpm ? osCommand.write(pathJoin(options.outputDir, 'pnpm-workspace.yaml'), ''): '', // Ensure node_modules directory is installed locally by creating local 'pnpm-workspace.yaml' file
osCommand.writeJson(pathJoin(options.outputDir, 'package.json'), { dependencies }),
osCommand.copy(lockFilePath, pathJoin(options.outputDir, this.packageManager.lockFile)),
osCommand.changeDirectory(options.outputDir),
this.packageManager.installCommand.join(' '),
isPnpm ? osCommand.remove(pathJoin(options.outputDir, 'node_modules', '.modules.yaml')) : '', // Remove '.modules.yaml' file which changes on each deployment
]);
}

Expand Down Expand Up @@ -310,13 +314,20 @@ 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: string): string {
if (this.osPlatform === 'win32') {
return `echo ^${stringifiedData}^ > "${filePath}"`;
if (!data) { // if `data` is empty, echo a blank line, otherwise the file will contain a `^` character
return `echo. > "${filePath}"`;
}
return `echo ^${data}^ > "${filePath}"`;
}

return `echo '${stringifiedData}' > "${filePath}"`;
return `echo '${data}' > "${filePath}"`;
}

public writeJson(filePath: string, data: any): string {
const stringifiedData = JSON.stringify(data);
return this.write(filePath, stringifiedData);
}

public copy(src: string, dest: string): string {
Expand All @@ -330,6 +341,14 @@ class OsCommand {
public changeDirectory(dir: string): string {
return `cd "${dir}"`;
}

public remove(filePath: string): string {
if (this.osPlatform === 'win32') {
return `del "${filePath}"`;
}

return `rm "${filePath}"`;
}
}

/**
Expand Down
6 changes: 3 additions & 3 deletions packages/@aws-cdk/aws-lambda-nodejs/lib/function.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,16 +51,16 @@ export interface NodejsFunctionProps extends lambda.FunctionOptions {
readonly awsSdkConnectionReuse?: boolean;

/**
* The path to the dependencies lock file (`yarn.lock` or `package-lock.json`).
* The path to the dependencies lock file (`yarn.lock`, `pnpm-lock.yaml` or `package-lock.json`).
*
* This will be used as the source for the volume mounted in the Docker
* container.
*
* Modules specified in `nodeModules` will be installed using the right
* installer (`npm` or `yarn`) along with this lock file.
* installer (`yarn`, `pnpm` or `npm`) along with this lock file.
*
* @default - the path is found by walking up parent directories searching for
* a `yarn.lock` or `package-lock.json` file
* a `yarn.lock`, `pnpm-lock.yaml` or `package-lock.json` file
*/
readonly depsLockFilePath?: string;

Expand Down
4 changes: 3 additions & 1 deletion packages/@aws-cdk/aws-lambda-nodejs/lib/package-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,9 @@ 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', '--config.node-linker=hoisted', '--config.package-import-method=clone-or-copy'] : ['pnpm', 'install', '--config.node-linker=hoisted', '--config.package-import-method=clone-or-copy'],
// --config.node-linker=hoisted to create flat node_modules without symlinks
// --config.package-import-method=clone-or-copy to avoid hardlinking packages from the store
runCommand: ['pnpm', 'exec'],
argsSeparator: '--',
});
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-lambda-nodejs/lib/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ export interface BundlingOptions extends DockerRunOptions {
* A custom bundling Docker image.
*
* This image should have esbuild installed globally. If you plan to use `nodeModules`
* it should also have `npm` or `yarn` depending on the lock file you're using.
* it should also have `npm`, `yarn` or `pnpm` depending on the lock file you're using.
*
* See https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-lambda-nodejs/lib/Dockerfile
* for the default image provided by @aws-cdk/aws-lambda-nodejs.
Expand Down
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-lambda-nodejs/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@
"@aws-cdk/integ-runner": "0.0.0",
"@aws-cdk/integ-tests": "0.0.0",
"@aws-cdk/pkglint": "0.0.0",
"@aws-cdk/triggers": "0.0.0",
"@types/jest": "^27.5.2",
"delay": "5.0.0",
"esbuild": "^0.16.6"
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-lambda-nodejs/test/bundling.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,7 @@ test('Detects pnpm-lock.yaml', () => {
assetHashType: AssetHashType.OUTPUT,
bundling: expect.objectContaining({
command: expect.arrayContaining([
expect.stringMatching(/pnpm-lock\.yaml.+pnpm install/),
expect.stringMatching(/echo '' > "\/asset-output\/pnpm-workspace.yaml\".+pnpm-lock\.yaml.+pnpm install --config.node-linker=hoisted --config.package-import-method=clone-or-copy && rm "\/asset-output\/node_modules\/.modules.yaml"/),
]),
}),
});
Expand Down
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-lambda-nodejs/test/docker.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ test('cache folders have the right permissions', () => {
'bash', '-c', [
'stat -c \'%a\' /tmp/npm-cache',
'stat -c \'%a\' /tmp/yarn-cache',
'stat -c \'%a\' /tmp/pnpm-cache',
].join(' && '),
]);
expect(proc.stdout.toString()).toMatch('777\n777');
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// eslint-disable-next-line import/no-extraneous-dependencies
import axios from 'axios';

export async function handler() {
await axios.get('https://www.google.com');
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{
"version": "22.0.0",
"files": {
"21fbb51d7b23f6a6c262b46a9caee79d744a3ac019fd45422d988b96d44b2a22": {
"source": {
"path": "PnpmTestDefaultTestDeployAssert397EDF83.template.json",
"packaging": "file"
},
"destinations": {
"current_account-current_region": {
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
"objectKey": "21fbb51d7b23f6a6c262b46a9caee79d744a3ac019fd45422d988b96d44b2a22.json",
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
}
}
}
},
"dockerImages": {}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
{
"Parameters": {
"BootstrapVersion": {
"Type": "AWS::SSM::Parameter::Value<String>",
"Default": "/cdk-bootstrap/hnb659fds/version",
"Description": "Version of the CDK Bootstrap resources in this environment, automatically retrieved from SSM Parameter Store. [cdk:skip]"
}
},
"Rules": {
"CheckBootstrapVersion": {
"Assertions": [
{
"Assert": {
"Fn::Not": [
{
"Fn::Contains": [
[
"1",
"2",
"3",
"4",
"5"
],
{
"Ref": "BootstrapVersion"
}
]
}
]
},
"AssertDescription": "CDK bootstrap stack version 6 required. Please run 'cdk bootstrap' with a recent version of the CDK CLI."
}
]
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
{
"version": "22.0.0",
"files": {
"58d2406e0a74fd42ac31e584a098bb581709c3de7bff389451c2a08db50c4383": {
"source": {
"path": "asset.58d2406e0a74fd42ac31e584a098bb581709c3de7bff389451c2a08db50c4383",
"packaging": "zip"
},
"destinations": {
"current_account-current_region": {
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
"objectKey": "58d2406e0a74fd42ac31e584a098bb581709c3de7bff389451c2a08db50c4383.zip",
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
}
}
},
"44fdff138ed916c94d14f276217febea5c4a2e1746518f7f620c37b22a6675b8": {
"source": {
"path": "asset.44fdff138ed916c94d14f276217febea5c4a2e1746518f7f620c37b22a6675b8",
"packaging": "zip"
},
"destinations": {
"current_account-current_region": {
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
"objectKey": "44fdff138ed916c94d14f276217febea5c4a2e1746518f7f620c37b22a6675b8.zip",
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
}
}
},
"c51cd09452e746f11796192b2e9025f8c82de145d16aeb2e2c66c2197ecbae26": {
"source": {
"path": "TestStack.template.json",
"packaging": "file"
},
"destinations": {
"current_account-current_region": {
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
"objectKey": "c51cd09452e746f11796192b2e9025f8c82de145d16aeb2e2c66c2197ecbae26.json",
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
}
}
}
},
"dockerImages": {}
}
Loading

0 comments on commit 7c752db

Please sign in to comment.