From 8e04ac4234923510bac1af8a372fccb6c772ab76 Mon Sep 17 00:00:00 2001 From: Jonathan Goldwasser Date: Tue, 18 Feb 2020 16:42:35 +0100 Subject: [PATCH 1/2] fix(lambda-nodejs): parcel is too big to bundle Stop bundling `parcel-bundler` and make it an undeclared peer dependency. Transform `build` function into a class. Fixes #6340 BREAKING CHANGE: `parcel-bundler` v1.x is now a peer dependency of `@aws-cdk/aws-lambda-nodejs`. Please add it to your `package.json`. --- package.json | 4 +- packages/@aws-cdk/aws-lambda-nodejs/README.md | 2 + .../@aws-cdk/aws-lambda-nodejs/lib/build.ts | 97 ---------------- .../@aws-cdk/aws-lambda-nodejs/lib/builder.ts | 108 ++++++++++++++++++ .../aws-lambda-nodejs/lib/function.ts | 5 +- .../@aws-cdk/aws-lambda-nodejs/package.json | 9 +- .../test/{build.test.ts => builder.test.ts} | 39 ++++++- .../aws-lambda-nodejs/test/function.test.ts | 22 ++-- 8 files changed, 164 insertions(+), 122 deletions(-) delete mode 100644 packages/@aws-cdk/aws-lambda-nodejs/lib/build.ts create mode 100644 packages/@aws-cdk/aws-lambda-nodejs/lib/builder.ts rename packages/@aws-cdk/aws-lambda-nodejs/test/{build.test.ts => builder.test.ts} (54%) diff --git a/package.json b/package.json index 20f79d432a0ea..acfc8502c54bc 100644 --- a/package.json +++ b/package.json @@ -55,9 +55,7 @@ "@aws-cdk/aws-ecr-assets/minimatch/**", "@aws-cdk/cx-api/semver", "@aws-cdk/cx-api/semver/**", - "@aws-cdk/cx-api/semver/**", - "@aws-cdk/aws-lambda-nodejs/parcel-bundler", - "@aws-cdk/aws-lambda-nodejs/parcel-bundler/**" + "@aws-cdk/cx-api/semver/**" ] } } diff --git a/packages/@aws-cdk/aws-lambda-nodejs/README.md b/packages/@aws-cdk/aws-lambda-nodejs/README.md index 49adc6ed4fb9b..503ecd091aad8 100644 --- a/packages/@aws-cdk/aws-lambda-nodejs/README.md +++ b/packages/@aws-cdk/aws-lambda-nodejs/README.md @@ -17,6 +17,8 @@ This library provides constructs for Node.js Lambda functions. +⚠️ This module has a peer dependency on [`parcel-bundler`](https://www.npmjs.com/package/parcel-bundler) v1.x. + ### Node.js Function Define a `NodejsFunction`: diff --git a/packages/@aws-cdk/aws-lambda-nodejs/lib/build.ts b/packages/@aws-cdk/aws-lambda-nodejs/lib/build.ts deleted file mode 100644 index e32bd1ec081f4..0000000000000 --- a/packages/@aws-cdk/aws-lambda-nodejs/lib/build.ts +++ /dev/null @@ -1,97 +0,0 @@ -import { spawnSync } from 'child_process'; -import * as fs from 'fs'; -import * as path from 'path'; -import { findPkgPath, updatePkg } from './util'; - -/** - * Build options - */ -export interface BuildOptions { - /** - * Entry file - */ - readonly entry: string; - - /** - * The output directory - */ - readonly outDir: string; - - /** - * Expose modules as UMD under this name - */ - readonly global: string; - - /** - * Minify - */ - readonly minify?: boolean; - - /** - * Include source maps - */ - readonly sourceMaps?: boolean; - - /** - * The cache directory - */ - readonly cacheDir?: string; - - /** - * The node version to use as target for Babel - */ - readonly nodeVersion?: string; -} - -/** - * Build with Parcel - */ -export function build(options: BuildOptions): void { - const pkgPath = findPkgPath(); - let originalPkg; - - try { - if (options.nodeVersion && pkgPath) { - // Update engines.node (Babel target) - originalPkg = updatePkg(pkgPath, { - engines: { node: `>= ${options.nodeVersion}` } - }); - } - - const args = [ - 'build', options.entry, - '--out-dir', options.outDir, - '--out-file', 'index.js', - '--global', options.global, - '--target', 'node', - '--bundle-node-modules', - '--log-level', '2', - !options.minify && '--no-minify', - !options.sourceMaps && '--no-source-maps', - ...options.cacheDir - ? ['--cache-dir', options.cacheDir] - : [], - ].filter(Boolean) as string[]; - - const parcelPkgPath = require.resolve('parcel-bundler/package.json'); // eslint-disable-line @typescript-eslint/no-require-imports - const parcelDir = path.dirname(parcelPkgPath); - const parcelPkg = require(parcelPkgPath); // eslint-disable-line @typescript-eslint/no-require-imports - const binPath = path.join(parcelDir, parcelPkg.bin.parcel); - - const parcel = spawnSync(binPath, args); - - if (parcel.error) { - throw parcel.error; - } - - if (parcel.status !== 0) { - throw new Error(parcel.stdout.toString().trim()); - } - } catch (err) { - throw new Error(`Failed to build file at ${options.entry}: ${err}`); - } finally { // Always restore package.json to original - if (pkgPath && originalPkg) { - fs.writeFileSync(pkgPath, originalPkg); - } - } -} diff --git a/packages/@aws-cdk/aws-lambda-nodejs/lib/builder.ts b/packages/@aws-cdk/aws-lambda-nodejs/lib/builder.ts new file mode 100644 index 0000000000000..86392f663be56 --- /dev/null +++ b/packages/@aws-cdk/aws-lambda-nodejs/lib/builder.ts @@ -0,0 +1,108 @@ +import { spawnSync } from 'child_process'; +import * as fs from 'fs'; +import * as path from 'path'; +import { findPkgPath, updatePkg } from './util'; + +/** + * Builder options + */ +export interface BuilderOptions { + /** + * Entry file + */ + readonly entry: string; + + /** + * The output directory + */ + readonly outDir: string; + + /** + * Expose modules as UMD under this name + */ + readonly global: string; + + /** + * Minify + */ + readonly minify?: boolean; + + /** + * Include source maps + */ + readonly sourceMaps?: boolean; + + /** + * The cache directory + */ + readonly cacheDir?: string; + + /** + * The node version to use as target for Babel + */ + readonly nodeVersion?: string; +} + +/** + * Builder + */ +export class Builder { + private readonly parcelBinPath: string; + + constructor(private readonly options: BuilderOptions) { + const parcelPkgPath = require.resolve('parcel-bundler/package.json'); // This will throw if `parcel-bundler` cannot be found + const parcelDir = path.dirname(parcelPkgPath); + const parcelPkg = JSON.parse(fs.readFileSync(parcelPkgPath, 'utf8')); + + if (!parcelPkg.version || !/^1\./.test(parcelPkg.version)) { // Peer dependency on parcel v1.x + throw new Error(`This module has a peer dependency on parcel-bundler v1.x. Got v${parcelPkg.version}.`); + } + + this.parcelBinPath = path.join(parcelDir, parcelPkg.bin.parcel); + } + + public build(): void { + const pkgPath = findPkgPath(); + let originalPkg; + + try { + if (this.options.nodeVersion && pkgPath) { + // Update engines.node (Babel target) + originalPkg = updatePkg(pkgPath, { + engines: { node: `>= ${this.options.nodeVersion}` } + }); + } + + const args = [ + 'build', this.options.entry, + '--out-dir', this.options.outDir, + '--out-file', 'index.js', + '--global', this.options.global, + '--target', 'node', + '--bundle-node-modules', + '--log-level', '2', + !this.options.minify && '--no-minify', + !this.options.sourceMaps && '--no-source-maps', + ...this.options.cacheDir + ? ['--cache-dir', this.options.cacheDir] + : [], + ].filter(Boolean) as string[]; + + const parcel = spawnSync(this.parcelBinPath, args); + + if (parcel.error) { + throw parcel.error; + } + + if (parcel.status !== 0) { + throw new Error(parcel.stdout.toString().trim()); + } + } catch (err) { + throw new Error(`Failed to build file at ${this.options.entry}: ${err}`); + } finally { // Always restore package.json to original + if (pkgPath && originalPkg) { + fs.writeFileSync(pkgPath, originalPkg); + } + } + } +} diff --git a/packages/@aws-cdk/aws-lambda-nodejs/lib/function.ts b/packages/@aws-cdk/aws-lambda-nodejs/lib/function.ts index 669c2bd9f055d..43f9e7f47ec71 100644 --- a/packages/@aws-cdk/aws-lambda-nodejs/lib/function.ts +++ b/packages/@aws-cdk/aws-lambda-nodejs/lib/function.ts @@ -3,7 +3,7 @@ import * as cdk from '@aws-cdk/core'; import * as crypto from 'crypto'; import * as fs from 'fs'; import * as path from 'path'; -import { build } from './build'; +import { Builder } from './builder'; import { nodeMajorVersion, parseStackTrace } from './util'; /** @@ -86,7 +86,7 @@ export class NodejsFunction extends lambda.Function { const runtime = props.runtime || defaultRunTime; // Build with Parcel - build({ + const builder = new Builder({ entry, outDir: handlerDir, global: handler, @@ -95,6 +95,7 @@ export class NodejsFunction extends lambda.Function { cacheDir: props.cacheDir, nodeVersion: extractVersion(runtime), }); + builder.build(); super(scope, id, { ...props, diff --git a/packages/@aws-cdk/aws-lambda-nodejs/package.json b/packages/@aws-cdk/aws-lambda-nodejs/package.json index 488f99065cb3a..adc32cb4e57ea 100644 --- a/packages/@aws-cdk/aws-lambda-nodejs/package.json +++ b/packages/@aws-cdk/aws-lambda-nodejs/package.json @@ -88,16 +88,13 @@ "cdk-build-tools": "1.24.0", "cdk-integ-tools": "1.24.0", "fs-extra": "^8.1.0", + "parcel-bundler": "^1.12.4", "pkglint": "1.24.0" }, "dependencies": { "@aws-cdk/aws-lambda": "1.24.0", - "@aws-cdk/core": "1.24.0", - "parcel-bundler": "^1.12.4" + "@aws-cdk/core": "1.24.0" }, - "bundledDependencies": [ - "parcel-bundler" - ], "homepage": "https://github.com/aws/aws-cdk", "peerDependencies": { "@aws-cdk/aws-lambda": "1.24.0", @@ -107,4 +104,4 @@ "node": ">= 10.3.0" }, "stability": "experimental" -} \ No newline at end of file +} diff --git a/packages/@aws-cdk/aws-lambda-nodejs/test/build.test.ts b/packages/@aws-cdk/aws-lambda-nodejs/test/builder.test.ts similarity index 54% rename from packages/@aws-cdk/aws-lambda-nodejs/test/build.test.ts rename to packages/@aws-cdk/aws-lambda-nodejs/test/builder.test.ts index 0dea4c73e70e7..c7ef0160ccf93 100644 --- a/packages/@aws-cdk/aws-lambda-nodejs/test/build.test.ts +++ b/packages/@aws-cdk/aws-lambda-nodejs/test/builder.test.ts @@ -1,5 +1,17 @@ import { spawnSync } from 'child_process'; -import { build } from '../lib/build'; +import * as fs from 'fs-extra'; +import { Builder } from '../lib/builder'; + +let parcelPkgPath: string; +let parcelPkg: Buffer; +beforeAll(() => { + parcelPkgPath = require.resolve('parcel-bundler/package.json'); + parcelPkg = fs.readFileSync(parcelPkgPath); +}); + +afterEach(() => { + fs.writeFileSync(parcelPkgPath, parcelPkg); +}); jest.mock('child_process', () => ({ spawnSync: jest.fn((_cmd: string, args: string[]) => { @@ -16,12 +28,13 @@ jest.mock('child_process', () => ({ })); test('calls parcel with the correct args', () => { - build({ + const builder = new Builder({ entry: 'entry', global: 'handler', outDir: 'out-dir', cacheDir: 'cache-dir', }); + builder.build(); expect(spawnSync).toHaveBeenCalledWith(expect.stringContaining('parcel-bundler'), expect.arrayContaining([ 'build', 'entry', @@ -38,17 +51,31 @@ test('calls parcel with the correct args', () => { }); test('throws in case of error', () => { - expect(() => build({ + const builder = new Builder({ entry: 'error', global: 'handler', outDir: 'out-dir' - })).toThrow('parcel-error'); + }); + expect(() => builder.build()).toThrow('parcel-error'); }); test('throws if status is not 0', () => { - expect(() => build({ + const builder = new Builder({ entry: 'status', global: 'handler', outDir: 'out-dir' - })).toThrow('status-error'); + }); + expect(() => builder.build()).toThrow('status-error'); +}); + +test('throws when parcel-bundler is not 1.x', () => { + fs.writeFileSync(parcelPkgPath, JSON.stringify({ + ...JSON.parse(parcelPkg.toString()), + version: '2.3.4' + })); + expect(() => new Builder({ + entry: 'entry', + global: 'handler', + outDir: 'out-dur' + })).toThrow(/This module has a peer dependency on parcel-bundler v1.x. Got v2.3.4./); }); diff --git a/packages/@aws-cdk/aws-lambda-nodejs/test/function.test.ts b/packages/@aws-cdk/aws-lambda-nodejs/test/function.test.ts index ca6f19dd21760..b6ad03ddf223a 100644 --- a/packages/@aws-cdk/aws-lambda-nodejs/test/function.test.ts +++ b/packages/@aws-cdk/aws-lambda-nodejs/test/function.test.ts @@ -4,13 +4,19 @@ import { Stack } from '@aws-cdk/core'; import * as fs from 'fs-extra'; import * as path from 'path'; import { NodejsFunction } from '../lib'; -import { build, BuildOptions } from '../lib/build'; +import { Builder, BuilderOptions } from '../lib/builder'; -jest.mock('../lib/build', () => ({ - build: jest.fn((options: BuildOptions) => { - require('fs-extra').ensureDirSync(options.outDir); // eslint-disable-line @typescript-eslint/no-require-imports - }) -})); +jest.mock('../lib/builder', () => { + return { + Builder: jest.fn().mockImplementation((options: BuilderOptions) => { + return { + build: jest.fn(() => { + require('fs-extra').ensureDirSync(options.outDir); // eslint-disable-line @typescript-eslint/no-require-imports + }) + }; + }) + }; +}); let stack: Stack; const buildDir = path.join(__dirname, '.build'); @@ -27,7 +33,7 @@ test('NodejsFunction with .ts handler', () => { // WHEN new NodejsFunction(stack, 'handler1'); - expect(build).toHaveBeenCalledWith(expect.objectContaining({ + expect(Builder).toHaveBeenCalledWith(expect.objectContaining({ entry: expect.stringContaining('function.test.handler1.ts'), // Automatically finds .ts handler file global: 'handler', outDir: expect.stringContaining(buildDir) @@ -43,7 +49,7 @@ test('NodejsFunction with .js handler', () => { new NodejsFunction(stack, 'handler2'); // THEN - expect(build).toHaveBeenCalledWith(expect.objectContaining({ + expect(Builder).toHaveBeenCalledWith(expect.objectContaining({ entry: expect.stringContaining('function.test.handler2.js'), // Automatically finds .ts handler file })); }); From c0c08871ee4d0de2aff5ca1353338c12fceb26f7 Mon Sep 17 00:00:00 2001 From: Jonathan Goldwasser Date: Wed, 19 Feb 2020 09:36:31 +0100 Subject: [PATCH 2/2] review comments --- packages/@aws-cdk/aws-lambda-nodejs/README.md | 9 ++++++++- packages/@aws-cdk/aws-lambda-nodejs/lib/builder.ts | 7 ++++++- packages/@aws-cdk/aws-lambda-nodejs/test/builder.test.ts | 2 +- 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/packages/@aws-cdk/aws-lambda-nodejs/README.md b/packages/@aws-cdk/aws-lambda-nodejs/README.md index 503ecd091aad8..18372808f23cd 100644 --- a/packages/@aws-cdk/aws-lambda-nodejs/README.md +++ b/packages/@aws-cdk/aws-lambda-nodejs/README.md @@ -17,7 +17,14 @@ This library provides constructs for Node.js Lambda functions. -⚠️ This module has a peer dependency on [`parcel-bundler`](https://www.npmjs.com/package/parcel-bundler) v1.x. +To use this module, you will need to add a dependency on `parcel-bundler` in your +`package.json`: + +``` +yarn add parcel-bundler@^1 +# or +npm install parcel-bundler@^1 +``` ### Node.js Function Define a `NodejsFunction`: diff --git a/packages/@aws-cdk/aws-lambda-nodejs/lib/builder.ts b/packages/@aws-cdk/aws-lambda-nodejs/lib/builder.ts index 86392f663be56..d215d4f19700c 100644 --- a/packages/@aws-cdk/aws-lambda-nodejs/lib/builder.ts +++ b/packages/@aws-cdk/aws-lambda-nodejs/lib/builder.ts @@ -50,7 +50,12 @@ export class Builder { private readonly parcelBinPath: string; constructor(private readonly options: BuilderOptions) { - const parcelPkgPath = require.resolve('parcel-bundler/package.json'); // This will throw if `parcel-bundler` cannot be found + let parcelPkgPath: string; + try { + parcelPkgPath = require.resolve('parcel-bundler/package.json'); // This will throw if `parcel-bundler` cannot be found + } catch (err) { + throw new Error('It looks like parcel-bundler is not installed. Please install v1.x of parcel-bundler with yarn or npm.'); + } const parcelDir = path.dirname(parcelPkgPath); const parcelPkg = JSON.parse(fs.readFileSync(parcelPkgPath, 'utf8')); diff --git a/packages/@aws-cdk/aws-lambda-nodejs/test/builder.test.ts b/packages/@aws-cdk/aws-lambda-nodejs/test/builder.test.ts index c7ef0160ccf93..fab7bbc418bde 100644 --- a/packages/@aws-cdk/aws-lambda-nodejs/test/builder.test.ts +++ b/packages/@aws-cdk/aws-lambda-nodejs/test/builder.test.ts @@ -1,5 +1,5 @@ import { spawnSync } from 'child_process'; -import * as fs from 'fs-extra'; +import * as fs from 'fs'; import { Builder } from '../lib/builder'; let parcelPkgPath: string;