From 3e4986c53bb4187e252a49692bee0c90e4cf64ac Mon Sep 17 00:00:00 2001 From: David Govea Date: Sun, 23 Jul 2017 01:35:36 -0700 Subject: [PATCH 01/12] Refactor zipService to assemble both dev and prod dependencies, and add failing test --- lib/plugins/package/lib/zipService.js | 27 +++++++----- lib/plugins/package/lib/zipService.test.js | 50 ++++++++++++++++++++++ 2 files changed, 67 insertions(+), 10 deletions(-) diff --git a/lib/plugins/package/lib/zipService.js b/lib/plugins/package/lib/zipService.js index 6837ff66d..f7fdcc700 100644 --- a/lib/plugins/package/lib/zipService.js +++ b/lib/plugins/package/lib/zipService.js @@ -161,17 +161,24 @@ function excludeNodeDevDependencies(servicePath) { // we added a catch which resolves so that npm commands with an exit code of 1 // (e.g. if the package.json is invalid) won't crash the dev dependency exclusion process - return childProcess.execAsync( - `npm ls --dev=true --parseable=true --silent >> ${nodeDevDepFile}`, - { cwd: dirWithPackageJson } - ).catch(() => BbPromise.resolve()); + return BbPromise.map(['dev', 'prod'], (env) => { + return childProcess.execAsync( + `npm ls --${env}=true --parseable=true --silent >> ${nodeDevDepFile}_${env}`, + { cwd: dirWithPackageJson } + ).catch(() => BbPromise.resolve()); + }); }) - .then(() => fs.readFileAsync(nodeDevDepFile)) - .then((fileContent) => { - const dependencies = _.compact( - (_.uniq(_.split(fileContent.toString('utf8'), '\n'))), - elem => elem.length > 0 - ); + .then(() => BbPromise.map(['dev', 'prod'], (env) => { + return fs.readFileAsync(`${nodeDevDepFile}_${env}`) + .then((fileContent) => _.compact( + (_.uniq(_.split(fileContent.toString('utf8'), '\n'))), + elem => elem.length > 0 + )); + })) + .then(([devDependencies/*, prodDependencies */]) => { + // const dependencies = _.difference(devDependencies, prodDependencies); + const dependencies = devDependencies; // Retain old functionality for failing test + const nodeModulesRegex = new RegExp(`${path.join('node_modules', path.sep)}.*`, 'g'); if (!_.isEmpty(dependencies)) { diff --git a/lib/plugins/package/lib/zipService.test.js b/lib/plugins/package/lib/zipService.test.js index a304bf861..12afb1099 100644 --- a/lib/plugins/package/lib/zipService.test.js +++ b/lib/plugins/package/lib/zipService.test.js @@ -369,6 +369,56 @@ describe('zipService', () => { expect(updatedParams.zipFileName).to.equal(params.zipFileName); }); }); + + it.only('should not include packages if in both dependencies and devDependencies', () => { + const filePaths = ['package.json', 'node_modules']; + + globbySyncStub.returns(filePaths); + execAsyncStub.resolves(); + + const devDepPaths = [ + `${servicePath}/node_modules/module-1`, + `${servicePath}/node_modules/module-2`, + ].join('\n'); + readFileAsyncStub.withArgs(sinon.match(/dev$/)).onCall(0).resolves(devDepPaths); + + const prodDepPaths = [ + `${servicePath}/node_modules/module-2`, + ]; + readFileAsyncStub.withArgs(sinon.match(/prod$/)).onCall(0).resolves(prodDepPaths); + + return expect(packagePlugin.excludeDevDependencies(params)).to.be + .fulfilled.then((updatedParams) => { + + expect(globbySyncStub).to.have.been.calledOnce; + expect(execAsyncStub).to.have.been.calledTwice; + expect(readFileAsyncStub).to.have.been.calledTwice; + expect(globbySyncStub).to.have.been + .calledWithExactly(['**/package.json'], { + cwd: packagePlugin.serverless.config.servicePath, + dot: true, + silent: true, + follow: true, + nosort: true, + }); + expect(execAsyncStub.args[0][0]).to + .match(/npm ls --dev=true --parseable=true --silent >> .+/); + expect(execAsyncStub.args[0][1].cwd).to + .match(/.+/); + expect(execAsyncStub.args[1][0]).to + .match(/npm ls --prod=true --parseable=true --silent >> .+/); + expect(execAsyncStub.args[1][1].cwd).to + .match(/.+/); + expect(updatedParams.exclude).to.deep.equal([ + 'user-defined-exclude-me', + 'node_modules/module-1/**', + ]); + expect(updatedParams.include).to.deep.equal([ + 'user-defined-include-me', + ]); + expect(updatedParams.zipFileName).to.equal(params.zipFileName); + }); + }); }); }); From 4104c918396b3822b9ffb5f54830c486494e4e73 Mon Sep 17 00:00:00 2001 From: David Govea Date: Sun, 23 Jul 2017 01:55:21 -0700 Subject: [PATCH 02/12] Enable dependency diffing, & remove `it.only()` from failing test (now passing) --- lib/plugins/package/lib/zipService.js | 5 ++--- lib/plugins/package/lib/zipService.test.js | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/plugins/package/lib/zipService.js b/lib/plugins/package/lib/zipService.js index f7fdcc700..d511e8128 100644 --- a/lib/plugins/package/lib/zipService.js +++ b/lib/plugins/package/lib/zipService.js @@ -175,10 +175,9 @@ function excludeNodeDevDependencies(servicePath) { elem => elem.length > 0 )); })) - .then(([devDependencies/*, prodDependencies */]) => { - // const dependencies = _.difference(devDependencies, prodDependencies); - const dependencies = devDependencies; // Retain old functionality for failing test + .then(([devDependencies, prodDependencies]) => { + const dependencies = _.difference(devDependencies, prodDependencies); const nodeModulesRegex = new RegExp(`${path.join('node_modules', path.sep)}.*`, 'g'); if (!_.isEmpty(dependencies)) { diff --git a/lib/plugins/package/lib/zipService.test.js b/lib/plugins/package/lib/zipService.test.js index 12afb1099..64190a12d 100644 --- a/lib/plugins/package/lib/zipService.test.js +++ b/lib/plugins/package/lib/zipService.test.js @@ -370,7 +370,7 @@ describe('zipService', () => { }); }); - it.only('should not include packages if in both dependencies and devDependencies', () => { + it('should not include packages if in both dependencies and devDependencies', () => { const filePaths = ['package.json', 'node_modules']; globbySyncStub.returns(filePaths); From 42c8c59bde265cda3c364c11a4e7a739693420c0 Mon Sep 17 00:00:00 2001 From: David Govea Date: Sun, 23 Jul 2017 01:56:26 -0700 Subject: [PATCH 03/12] double stub call counts for new code path --- lib/plugins/package/lib/zipService.test.js | 63 +++++++++++++++++----- 1 file changed, 49 insertions(+), 14 deletions(-) diff --git a/lib/plugins/package/lib/zipService.test.js b/lib/plugins/package/lib/zipService.test.js index 64190a12d..8b6e88a67 100644 --- a/lib/plugins/package/lib/zipService.test.js +++ b/lib/plugins/package/lib/zipService.test.js @@ -136,8 +136,8 @@ describe('zipService', () => { return expect(packagePlugin.excludeDevDependencies(params)).to.be .fulfilled.then((updatedParams) => { expect(globbySyncStub).to.have.been.calledOnce; - expect(execAsyncStub).to.have.been.calledOnce; - expect(readFileAsyncStub).to.have.been.calledOnce; + expect(execAsyncStub).to.have.been.calledTwice; + expect(readFileAsyncStub).to.have.been.calledTwice; expect(globbySyncStub).to.have.been .calledWithExactly(['**/package.json'], { cwd: packagePlugin.serverless.config.servicePath, @@ -150,6 +150,10 @@ describe('zipService', () => { .match(/npm ls --dev=true --parseable=true --silent >> .+/); expect(execAsyncStub.args[0][1].cwd).to .match(/.+/); + expect(execAsyncStub.args[1][0]).to + .match(/npm ls --prod=true --parseable=true --silent >> .+/); + expect(execAsyncStub.args[1][1].cwd).to + .match(/.+/); expect(updatedParams.exclude).to.deep.equal([ 'user-defined-exclude-me', ]); @@ -188,8 +192,8 @@ describe('zipService', () => { return expect(packagePlugin.excludeDevDependencies(params)).to.be .fulfilled.then((updatedParams) => { expect(globbySyncStub).to.been.calledOnce; - expect(execAsyncStub).to.have.been.calledOnce; - expect(readFileAsyncStub).to.have.been.calledOnce; + expect(execAsyncStub).to.have.been.calledTwice; + expect(readFileAsyncStub).to.have.been.calledTwice; expect(updatedParams.exclude).to.deep.equal([ 'user-defined-exclude-me', ]); @@ -215,8 +219,11 @@ describe('zipService', () => { globbySyncStub.returns(filePaths); execAsyncStub.onCall(0).resolves(); - execAsyncStub.onCall(1).rejects(); - execAsyncStub.onCall(2).resolves(); + execAsyncStub.onCall(1).resolves(); + execAsyncStub.onCall(2).rejects(); + execAsyncStub.onCall(3).rejects(); + execAsyncStub.onCall(4).resolves(); + execAsyncStub.onCall(5).resolves(); const depPaths = [ `${servicePath}/node_modules/module-1`, `${servicePath}/node_modules/module-2`, @@ -228,8 +235,8 @@ describe('zipService', () => { return expect(packagePlugin.excludeDevDependencies(params)).to.be .fulfilled.then((updatedParams) => { expect(globbySyncStub).to.have.been.calledOnce; - expect(execAsyncStub.callCount).to.equal(3); - expect(readFileAsyncStub).to.have.been.calledOnce; + expect(execAsyncStub.callCount).to.equal(6); + expect(readFileAsyncStub).to.have.been.calledTwice; expect(globbySyncStub).to.have.been .calledWithExactly(['**/package.json'], { cwd: packagePlugin.serverless.config.servicePath, @@ -243,13 +250,25 @@ describe('zipService', () => { expect(execAsyncStub.args[0][1].cwd).to .match(/.+/); expect(execAsyncStub.args[1][0]).to - .match(/npm ls --dev=true --parseable=true --silent >> .+/); + .match(/npm ls --prod=true --parseable=true --silent >> .+/); expect(execAsyncStub.args[1][1].cwd).to .match(/.+/); expect(execAsyncStub.args[2][0]).to .match(/npm ls --dev=true --parseable=true --silent >> .+/); expect(execAsyncStub.args[2][1].cwd).to .match(/.+/); + expect(execAsyncStub.args[3][0]).to + .match(/npm ls --prod=true --parseable=true --silent >> .+/); + expect(execAsyncStub.args[3][1].cwd).to + .match(/.+/); + expect(execAsyncStub.args[4][0]).to + .match(/npm ls --dev=true --parseable=true --silent >> .+/); + expect(execAsyncStub.args[4][1].cwd).to + .match(/.+/); + expect(execAsyncStub.args[5][0]).to + .match(/npm ls --prod=true --parseable=true --silent >> .+/); + expect(execAsyncStub.args[5][1].cwd).to + .match(/.+/); expect(updatedParams.exclude).to.deep.equal([ 'user-defined-exclude-me', 'node_modules/module-1/**', @@ -279,8 +298,8 @@ describe('zipService', () => { return expect(packagePlugin.excludeDevDependencies(params)).to.be .fulfilled.then((updatedParams) => { expect(globbySyncStub).to.have.been.calledOnce; - expect(execAsyncStub).to.have.been.calledOnce; - expect(readFileAsyncStub).to.have.been.calledOnce; + expect(execAsyncStub).to.have.been.calledTwice; + expect(readFileAsyncStub).to.have.been.calledTwice; expect(globbySyncStub).to.have.been .calledWithExactly(['**/package.json'], { cwd: packagePlugin.serverless.config.servicePath, @@ -293,6 +312,10 @@ describe('zipService', () => { .match(/npm ls --dev=true --parseable=true --silent >> .+/); expect(execAsyncStub.args[0][1].cwd).to .match(/.+/); + expect(execAsyncStub.args[1][0]).to + .match(/npm ls --prod=true --parseable=true --silent >> .+/); + expect(execAsyncStub.args[1][1].cwd).to + .match(/.+/); expect(updatedParams.exclude).to.deep.equal([ 'user-defined-exclude-me', 'node_modules/module-1/**', @@ -332,8 +355,8 @@ describe('zipService', () => { return expect(packagePlugin.excludeDevDependencies(params)).to.be .fulfilled.then((updatedParams) => { expect(globbySyncStub).to.have.been.calledOnce; - expect(execAsyncStub.callCount).to.equal(3); - expect(readFileAsyncStub).to.have.been.calledOnce; + expect(execAsyncStub.callCount).to.equal(6); + expect(readFileAsyncStub).to.have.been.calledTwice; expect(globbySyncStub).to.have.been .calledWithExactly(['**/package.json'], { cwd: packagePlugin.serverless.config.servicePath, @@ -347,13 +370,25 @@ describe('zipService', () => { expect(execAsyncStub.args[0][1].cwd).to .match(/.+/); expect(execAsyncStub.args[1][0]).to - .match(/npm ls --dev=true --parseable=true --silent >> .+/); + .match(/npm ls --prod=true --parseable=true --silent >> .+/); expect(execAsyncStub.args[1][1].cwd).to .match(/.+/); expect(execAsyncStub.args[2][0]).to .match(/npm ls --dev=true --parseable=true --silent >> .+/); expect(execAsyncStub.args[2][1].cwd).to .match(/.+/); + expect(execAsyncStub.args[3][0]).to + .match(/npm ls --prod=true --parseable=true --silent >> .+/); + expect(execAsyncStub.args[3][1].cwd).to + .match(/.+/); + expect(execAsyncStub.args[4][0]).to + .match(/npm ls --dev=true --parseable=true --silent >> .+/); + expect(execAsyncStub.args[4][1].cwd).to + .match(/.+/); + expect(execAsyncStub.args[5][0]).to + .match(/npm ls --prod=true --parseable=true --silent >> .+/); + expect(execAsyncStub.args[5][1].cwd).to + .match(/.+/); expect(updatedParams.exclude).to.deep.equal([ 'user-defined-exclude-me', 'node_modules/module-1/**', From 56852b74bad68bfb54e61763528b4061e29db9ae Mon Sep 17 00:00:00 2001 From: David Govea Date: Sun, 23 Jul 2017 01:56:58 -0700 Subject: [PATCH 04/12] Adjust data returned from "npm ls" stubs in dev & prod modes. All tests passing --- lib/plugins/package/lib/zipService.test.js | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/plugins/package/lib/zipService.test.js b/lib/plugins/package/lib/zipService.test.js index 8b6e88a67..e2bb83d49 100644 --- a/lib/plugins/package/lib/zipService.test.js +++ b/lib/plugins/package/lib/zipService.test.js @@ -230,7 +230,8 @@ describe('zipService', () => { `${servicePath}/1st/2nd/node_modules/module-1`, `${servicePath}/1st/2nd/node_modules/module-2`, ].join('\n'); - readFileAsyncStub.resolves(depPaths); + readFileAsyncStub.withArgs(sinon.match(/dev$/)).resolves(depPaths); + readFileAsyncStub.withArgs(sinon.match(/prod$/)).resolves([]); return expect(packagePlugin.excludeDevDependencies(params)).to.be .fulfilled.then((updatedParams) => { @@ -293,7 +294,8 @@ describe('zipService', () => { `${servicePath}/node_modules/module-1`, `${servicePath}/node_modules/module-2`, ].join('\n'); - readFileAsyncStub.resolves(depPaths); + readFileAsyncStub.withArgs(sinon.match(/dev$/)).resolves(depPaths); + readFileAsyncStub.withArgs(sinon.match(/prod$/)).resolves([]); return expect(packagePlugin.excludeDevDependencies(params)).to.be .fulfilled.then((updatedParams) => { @@ -351,6 +353,8 @@ describe('zipService', () => { `${servicePath}/1st/2nd/node_modules/module-2`, ].join('\n'); readFileAsyncStub.resolves(depPaths); + readFileAsyncStub.withArgs(sinon.match(/dev$/)).resolves(depPaths); + readFileAsyncStub.withArgs(sinon.match(/prod$/)).resolves([]); return expect(packagePlugin.excludeDevDependencies(params)).to.be .fulfilled.then((updatedParams) => { @@ -415,12 +419,12 @@ describe('zipService', () => { `${servicePath}/node_modules/module-1`, `${servicePath}/node_modules/module-2`, ].join('\n'); - readFileAsyncStub.withArgs(sinon.match(/dev$/)).onCall(0).resolves(devDepPaths); + readFileAsyncStub.withArgs(sinon.match(/dev$/)).resolves(devDepPaths); const prodDepPaths = [ `${servicePath}/node_modules/module-2`, ]; - readFileAsyncStub.withArgs(sinon.match(/prod$/)).onCall(0).resolves(prodDepPaths); + readFileAsyncStub.withArgs(sinon.match(/prod$/)).resolves(prodDepPaths); return expect(packagePlugin.excludeDevDependencies(params)).to.be .fulfilled.then((updatedParams) => { From e3582b6855da62fedfc95434fd5d92b97315c1ee Mon Sep 17 00:00:00 2001 From: David Govea Date: Sun, 23 Jul 2017 02:23:59 -0700 Subject: [PATCH 05/12] ESLint errors --- lib/plugins/package/lib/zipService.js | 17 ++++++++--------- lib/plugins/package/lib/zipService.test.js | 1 - 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/lib/plugins/package/lib/zipService.js b/lib/plugins/package/lib/zipService.js index d511e8128..40961ff36 100644 --- a/lib/plugins/package/lib/zipService.js +++ b/lib/plugins/package/lib/zipService.js @@ -161,22 +161,21 @@ function excludeNodeDevDependencies(servicePath) { // we added a catch which resolves so that npm commands with an exit code of 1 // (e.g. if the package.json is invalid) won't crash the dev dependency exclusion process - return BbPromise.map(['dev', 'prod'], (env) => { - return childProcess.execAsync( + return BbPromise.map(['dev', 'prod'], (env) => + childProcess.execAsync( `npm ls --${env}=true --parseable=true --silent >> ${nodeDevDepFile}_${env}`, { cwd: dirWithPackageJson } - ).catch(() => BbPromise.resolve()); - }); + ).catch(() => BbPromise.resolve()) + ); }) - .then(() => BbPromise.map(['dev', 'prod'], (env) => { - return fs.readFileAsync(`${nodeDevDepFile}_${env}`) + .then(() => BbPromise.map(['dev', 'prod'], (env) => + fs.readFileAsync(`${nodeDevDepFile}_${env}`) .then((fileContent) => _.compact( (_.uniq(_.split(fileContent.toString('utf8'), '\n'))), elem => elem.length > 0 - )); - })) + )) + )) .then(([devDependencies, prodDependencies]) => { - const dependencies = _.difference(devDependencies, prodDependencies); const nodeModulesRegex = new RegExp(`${path.join('node_modules', path.sep)}.*`, 'g'); diff --git a/lib/plugins/package/lib/zipService.test.js b/lib/plugins/package/lib/zipService.test.js index e2bb83d49..dc86efee6 100644 --- a/lib/plugins/package/lib/zipService.test.js +++ b/lib/plugins/package/lib/zipService.test.js @@ -428,7 +428,6 @@ describe('zipService', () => { return expect(packagePlugin.excludeDevDependencies(params)).to.be .fulfilled.then((updatedParams) => { - expect(globbySyncStub).to.have.been.calledOnce; expect(execAsyncStub).to.have.been.calledTwice; expect(readFileAsyncStub).to.have.been.calledTwice; From b095a286b51c2be37dc2d3ace6ff955f29e988ef Mon Sep 17 00:00:00 2001 From: David Govea Date: Sun, 23 Jul 2017 02:28:59 -0700 Subject: [PATCH 06/12] Remove destructuring :( breaking builds on node 4-5 --- lib/plugins/package/lib/zipService.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/plugins/package/lib/zipService.js b/lib/plugins/package/lib/zipService.js index 40961ff36..fab5c7d83 100644 --- a/lib/plugins/package/lib/zipService.js +++ b/lib/plugins/package/lib/zipService.js @@ -175,7 +175,10 @@ function excludeNodeDevDependencies(servicePath) { elem => elem.length > 0 )) )) - .then(([devDependencies, prodDependencies]) => { + .then((devDependenciesAndProdDependencies) => { + const devDependencies = devDependenciesAndProdDependencies[0]; + const prodDependencies = devDependenciesAndProdDependencies[1]; + const dependencies = _.difference(devDependencies, prodDependencies); const nodeModulesRegex = new RegExp(`${path.join('node_modules', path.sep)}.*`, 'g'); From d73fff689c6a097e4930b65bf816564c6d0d47ff Mon Sep 17 00:00:00 2001 From: Philipp Muens Date: Wed, 26 Jul 2017 10:51:35 +0200 Subject: [PATCH 07/12] Refactor usage of temporary dependency files --- lib/plugins/package/lib/zipService.js | 30 ++++++++++++++------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/lib/plugins/package/lib/zipService.js b/lib/plugins/package/lib/zipService.js index fab5c7d83..643165a59 100644 --- a/lib/plugins/package/lib/zipService.js +++ b/lib/plugins/package/lib/zipService.js @@ -125,11 +125,11 @@ function excludeNodeDevDependencies(servicePath) { exclude: [], }; - // the file where we'll write the dependencies into - const nodeDevDepFile = path.join( - os.tmpdir(), - `node-dev-dependencies-${crypto.randomBytes(8).toString('hex')}` - ); + // the files where we'll write the dependencies into + const tmpDir = os.tmpdir(); + const randHash = crypto.randomBytes(8).toString('hex'); + const nodeDevDepFile = path.join(tmpDir, `node-dependencies-${randHash}-dev`); + const nodeProdDepFile = path.join(tmpDir, `node-dependencies-${randHash}-prod`); try { const packageJsonFilePaths = globby.sync([ @@ -161,20 +161,22 @@ function excludeNodeDevDependencies(servicePath) { // we added a catch which resolves so that npm commands with an exit code of 1 // (e.g. if the package.json is invalid) won't crash the dev dependency exclusion process - return BbPromise.map(['dev', 'prod'], (env) => - childProcess.execAsync( - `npm ls --${env}=true --parseable=true --silent >> ${nodeDevDepFile}_${env}`, + return BbPromise.map(['dev', 'prod'], (env) => { + const depFile = env === 'dev' ? nodeDevDepFile : nodeProdDepFile; + return childProcess.execAsync( + `npm ls --${env}=true --parseable=true --silent >> ${depFile}`, { cwd: dirWithPackageJson } - ).catch(() => BbPromise.resolve()) - ); + ).catch(() => BbPromise.resolve()); + }); }) - .then(() => BbPromise.map(['dev', 'prod'], (env) => - fs.readFileAsync(`${nodeDevDepFile}_${env}`) + .then(() => BbPromise.map(['dev', 'prod'], (env) => { + const depFile = env === 'dev' ? nodeDevDepFile : nodeProdDepFile; + return fs.readFileAsync(depFile) .then((fileContent) => _.compact( (_.uniq(_.split(fileContent.toString('utf8'), '\n'))), elem => elem.length > 0 - )) - )) + )); + })) .then((devDependenciesAndProdDependencies) => { const devDependencies = devDependenciesAndProdDependencies[0]; const prodDependencies = devDependenciesAndProdDependencies[1]; From a54330aa0c1201eba6e48456dddc81bc4b41f574 Mon Sep 17 00:00:00 2001 From: Philipp Muens Date: Wed, 26 Jul 2017 10:53:18 +0200 Subject: [PATCH 08/12] Switch to mapSeries to ensure sequential execution / order of mapping --- lib/plugins/package/lib/zipService.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/plugins/package/lib/zipService.js b/lib/plugins/package/lib/zipService.js index 643165a59..f4bc5b0cc 100644 --- a/lib/plugins/package/lib/zipService.js +++ b/lib/plugins/package/lib/zipService.js @@ -161,7 +161,7 @@ function excludeNodeDevDependencies(servicePath) { // we added a catch which resolves so that npm commands with an exit code of 1 // (e.g. if the package.json is invalid) won't crash the dev dependency exclusion process - return BbPromise.map(['dev', 'prod'], (env) => { + return BbPromise.mapSeries(['dev', 'prod'], (env) => { const depFile = env === 'dev' ? nodeDevDepFile : nodeProdDepFile; return childProcess.execAsync( `npm ls --${env}=true --parseable=true --silent >> ${depFile}`, @@ -169,7 +169,7 @@ function excludeNodeDevDependencies(servicePath) { ).catch(() => BbPromise.resolve()); }); }) - .then(() => BbPromise.map(['dev', 'prod'], (env) => { + .then(() => BbPromise.mapSeries(['dev', 'prod'], (env) => { const depFile = env === 'dev' ? nodeDevDepFile : nodeProdDepFile; return fs.readFileAsync(depFile) .then((fileContent) => _.compact( From b9de80b5aacd50991f5a5f674b9761091be40439 Mon Sep 17 00:00:00 2001 From: Philipp Muens Date: Wed, 26 Jul 2017 12:14:44 +0200 Subject: [PATCH 09/12] Rename variables / add important note about _.difference --- lib/plugins/package/lib/zipService.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/plugins/package/lib/zipService.js b/lib/plugins/package/lib/zipService.js index f4bc5b0cc..692e04961 100644 --- a/lib/plugins/package/lib/zipService.js +++ b/lib/plugins/package/lib/zipService.js @@ -177,10 +177,11 @@ function excludeNodeDevDependencies(servicePath) { elem => elem.length > 0 )); })) - .then((devDependenciesAndProdDependencies) => { - const devDependencies = devDependenciesAndProdDependencies[0]; - const prodDependencies = devDependenciesAndProdDependencies[1]; + .then((devAndProDependencies) => { + const devDependencies = devAndProDependencies[0]; + const prodDependencies = devAndProDependencies[1]; + // NOTE: the order for _.difference is important const dependencies = _.difference(devDependencies, prodDependencies); const nodeModulesRegex = new RegExp(`${path.join('node_modules', path.sep)}.*`, 'g'); From 2802281e6d91de3969393547b279a78cc852d2b6 Mon Sep 17 00:00:00 2001 From: Philipp Muens Date: Wed, 26 Jul 2017 12:17:26 +0200 Subject: [PATCH 10/12] Remove unnecessary mapSeries usage / add note about mapSeries usage --- lib/plugins/package/lib/zipService.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/plugins/package/lib/zipService.js b/lib/plugins/package/lib/zipService.js index 692e04961..ef9dcc593 100644 --- a/lib/plugins/package/lib/zipService.js +++ b/lib/plugins/package/lib/zipService.js @@ -161,7 +161,7 @@ function excludeNodeDevDependencies(servicePath) { // we added a catch which resolves so that npm commands with an exit code of 1 // (e.g. if the package.json is invalid) won't crash the dev dependency exclusion process - return BbPromise.mapSeries(['dev', 'prod'], (env) => { + return BbPromise.map(['dev', 'prod'], (env) => { const depFile = env === 'dev' ? nodeDevDepFile : nodeProdDepFile; return childProcess.execAsync( `npm ls --${env}=true --parseable=true --silent >> ${depFile}`, @@ -169,6 +169,7 @@ function excludeNodeDevDependencies(servicePath) { ).catch(() => BbPromise.resolve()); }); }) + // NOTE: using mapSeries here for a sequential computation (w/o race conditions) .then(() => BbPromise.mapSeries(['dev', 'prod'], (env) => { const depFile = env === 'dev' ? nodeDevDepFile : nodeProdDepFile; return fs.readFileAsync(depFile) From 1540b4ed2dea3f453b1709c0cea86542677683af Mon Sep 17 00:00:00 2001 From: Philipp Muens Date: Wed, 26 Jul 2017 12:27:26 +0200 Subject: [PATCH 11/12] Add CLI output about dev dependency exclusion --- lib/plugins/package/lib/zipService.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/plugins/package/lib/zipService.js b/lib/plugins/package/lib/zipService.js index ef9dcc593..16b3c6243 100644 --- a/lib/plugins/package/lib/zipService.js +++ b/lib/plugins/package/lib/zipService.js @@ -35,6 +35,8 @@ module.exports = { } if (excludeDevDependencies) { + this.serverless.cli.log('Excluding development dependencies...'); + return BbPromise.bind(this) .then(() => excludeNodeDevDependencies(servicePath)) .then((exAndInNode) => { From 1acb4c5e445c08bb9849b05d01592bfb1620f525 Mon Sep 17 00:00:00 2001 From: Philipp Muens Date: Wed, 26 Jul 2017 12:59:31 +0200 Subject: [PATCH 12/12] Add catch-all for readFile code / test hardening --- lib/plugins/package/lib/zipService.js | 2 +- lib/plugins/package/lib/zipService.test.js | 28 ++++++++++++++++++++-- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/lib/plugins/package/lib/zipService.js b/lib/plugins/package/lib/zipService.js index 16b3c6243..582f683cc 100644 --- a/lib/plugins/package/lib/zipService.js +++ b/lib/plugins/package/lib/zipService.js @@ -178,7 +178,7 @@ function excludeNodeDevDependencies(servicePath) { .then((fileContent) => _.compact( (_.uniq(_.split(fileContent.toString('utf8'), '\n'))), elem => elem.length > 0 - )); + )).catch(() => BbPromise.resolve()); })) .then((devAndProDependencies) => { const devDependencies = devAndProDependencies[0]; diff --git a/lib/plugins/package/lib/zipService.test.js b/lib/plugins/package/lib/zipService.test.js index dc86efee6..8727ad496 100644 --- a/lib/plugins/package/lib/zipService.test.js +++ b/lib/plugins/package/lib/zipService.test.js @@ -182,12 +182,36 @@ describe('zipService', () => { }); }); - it('should return excludes and includes if a Promise is rejected in the chain', () => { + it('should return excludes and includes if a exec Promise is rejected', () => { + const filePaths = ['package.json', 'node_modules']; + + globbySyncStub.returns(filePaths); + execAsyncStub.onCall(0).resolves(); + execAsyncStub.onCall(1).rejects(); + readFileAsyncStub.resolves(); + + return expect(packagePlugin.excludeDevDependencies(params)).to.be + .fulfilled.then((updatedParams) => { + expect(globbySyncStub).to.been.calledOnce; + expect(execAsyncStub).to.have.been.calledTwice; + expect(readFileAsyncStub).to.have.been.calledTwice; + expect(updatedParams.exclude).to.deep.equal([ + 'user-defined-exclude-me', + ]); + expect(updatedParams.include).to.deep.equal([ + 'user-defined-include-me', + ]); + expect(updatedParams.zipFileName).to.equal(params.zipFileName); + }); + }); + + it('should return excludes and includes if a readFile Promise is rejected', () => { const filePaths = ['package.json', 'node_modules']; globbySyncStub.returns(filePaths); execAsyncStub.resolves(); - readFileAsyncStub.rejects(); + readFileAsyncStub.onCall(0).resolves(); + readFileAsyncStub.onCall(1).rejects(); return expect(packagePlugin.excludeDevDependencies(params)).to.be .fulfilled.then((updatedParams) => {