Merge pull request #3924 from medikoo/improve-packaging-extensibility

Improve configurability and performance of package plugin

lib/plugins/package/lib/packageService.js

@@ -2,6 +2,7 @@
 
 const BbPromise = require('bluebird');
 const path = require('path');
+const globby = require('globby');
 const _ = require('lodash');
 
 module.exports = {
@@ -60,19 +61,19 @@ module.exports = {
   },
 
   packageAll() {
-    const exclude = this.getExcludes();
-    const include = this.getIncludes();
     const zipFileName = `${this.serverless.service.service}.zip`;
 
-    return this.zipService(exclude, include, zipFileName).then(filePath => {
-      // only set the default artifact for backward-compatibility
-      // when no explicit artifact is defined
-      if (!this.serverless.service.package.artifact) {
-        this.serverless.service.package.artifact = filePath;
-        this.serverless.service.artifact = filePath;
-      }
-      return filePath;
-    });
+    return this.resolveFilePathsAll().then(filePaths =>
+      this.zipFiles(filePaths, zipFileName).then(filePath => {
+        // only set the default artifact for backward-compatibility
+        // when no explicit artifact is defined
+        if (!this.serverless.service.package.artifact) {
+          this.serverless.service.package.artifact = filePath;
+          this.serverless.service.artifact = filePath;
+        }
+        return filePath;
+      })
+    );
   },
 
   packageFunction(functionName) {
@@ -94,15 +95,62 @@ module.exports = {
       return BbPromise.resolve(filePath);
     }
 
-    const exclude = this.getExcludes(funcPackageConfig.exclude);
-    const include = this.getIncludes(funcPackageConfig.include);
     const zipFileName = `${functionName}.zip`;
 
-    return this.zipService(exclude, include, zipFileName).then(artifactPath => {
-      functionObject.package = {
-        artifact: artifactPath,
-      };
-      return artifactPath;
+    return this.resolveFilePathsFunction(functionName).then(filePaths =>
+      this.zipFiles(filePaths, zipFileName).then(artifactPath => {
+        functionObject.package = {
+          artifact: artifactPath,
+        };
+        return artifactPath;
+      })
+    );
+  },
+
+  resolveFilePathsAll() {
+    const params = { exclude: this.getExcludes(), include: this.getIncludes() };
+    return this.excludeDevDependencies(params).then(() =>
+      this.resolveFilePathsFromPatterns(params));
+  },
+
+  resolveFilePathsFunction(functionName) {
+    const functionObject = this.serverless.service.getFunction(functionName);
+    const funcPackageConfig = functionObject.package || {};
+
+    const params = {
+      exclude: this.getExcludes(funcPackageConfig.exclude),
+      include: this.getIncludes(funcPackageConfig.include),
+    };
+    return this.excludeDevDependencies(params).then(() =>
+      this.resolveFilePathsFromPatterns(params));
+  },
+
+  resolveFilePathsFromPatterns(params) {
+    const patterns = ['**'];
+
+    params.exclude.forEach((pattern) => {
+      if (pattern.charAt(0) !== '!') {
+        patterns.push(`!${pattern}`);
+      } else {
+        patterns.push(pattern.substring(1));
+      }
+    });
+
+    // push the include globs to the end of the array
+    // (files and folders will be re-added again even if they were excluded beforehand)
+    params.include.forEach((pattern) => {
+      patterns.push(pattern);
+    });
+
+    return globby(patterns, {
+      cwd: this.serverless.config.servicePath,
+      dot: true,
+      silent: true,
+      follow: true,
+      nodir: true,
+    }).then(filePaths => {
+      if (filePaths.length !== 0) return filePaths;
+      throw new this.serverless.classes.Error('No file matches include / exclude patterns');
     });
   },
 };

lib/plugins/package/lib/packageService.test.js

@@ -226,24 +226,29 @@ describe('#packageService()', () => {
   describe('#packageAll()', () => {
     const exclude = ['test-exclude'];
     const include = ['test-include'];
+    const files = [];
     const artifactFilePath = '/some/fake/path/test-artifact.zip';
     let getExcludesStub;
     let getIncludesStub;
-    let zipServiceStub;
+    let resolveFilePathsFromPatternsStub;
+    let zipFilesStub;
 
     beforeEach(() => {
       getExcludesStub = sinon
         .stub(packagePlugin, 'getExcludes').returns(exclude);
       getIncludesStub = sinon
         .stub(packagePlugin, 'getIncludes').returns(include);
-      zipServiceStub = sinon
-        .stub(packagePlugin, 'zipService').resolves(artifactFilePath);
+      resolveFilePathsFromPatternsStub = sinon
+        .stub(packagePlugin, 'resolveFilePathsFromPatterns').returns(files);
+      zipFilesStub = sinon
+        .stub(packagePlugin, 'zipFiles').resolves(artifactFilePath);
     });
 
     afterEach(() => {
       packagePlugin.getExcludes.restore();
       packagePlugin.getIncludes.restore();
-      packagePlugin.zipService.restore();
+      packagePlugin.resolveFilePathsFromPatterns.restore();
+      packagePlugin.zipFiles.restore();
     });
 
     it('should call zipService with settings', () => {
@@ -256,10 +261,10 @@ describe('#packageService()', () => {
       .then(() => BbPromise.all([
         expect(getExcludesStub).to.be.calledOnce,
         expect(getIncludesStub).to.be.calledOnce,
-        expect(zipServiceStub).to.be.calledOnce,
-        expect(zipServiceStub).to.have.been.calledWithExactly(
-          exclude,
-          include,
+        expect(resolveFilePathsFromPatternsStub).to.be.calledOnce,
+        expect(zipFilesStub).to.be.calledOnce,
+        expect(zipFilesStub).to.have.been.calledWithExactly(
+          files,
           zipFileName
         ),
       ]));
@@ -269,24 +274,29 @@ describe('#packageService()', () => {
   describe('#packageFunction()', () => {
     const exclude = ['test-exclude'];
     const include = ['test-include'];
+    const files = [];
     const artifactFilePath = '/some/fake/path/test-artifact.zip';
     let getExcludesStub;
     let getIncludesStub;
-    let zipServiceStub;
+    let resolveFilePathsFromPatternsStub;
+    let zipFilesStub;
 
     beforeEach(() => {
       getExcludesStub = sinon
         .stub(packagePlugin, 'getExcludes').returns(exclude);
       getIncludesStub = sinon
         .stub(packagePlugin, 'getIncludes').returns(include);
-      zipServiceStub = sinon
-        .stub(packagePlugin, 'zipService').resolves(artifactFilePath);
+      resolveFilePathsFromPatternsStub = sinon
+        .stub(packagePlugin, 'resolveFilePathsFromPatterns').returns(files);
+      zipFilesStub = sinon
+        .stub(packagePlugin, 'zipFiles').resolves(artifactFilePath);
     });
 
     afterEach(() => {
       packagePlugin.getExcludes.restore();
       packagePlugin.getIncludes.restore();
-      packagePlugin.zipService.restore();
+      packagePlugin.resolveFilePathsFromPatterns.restore();
+      packagePlugin.zipFiles.restore();
     });
 
     it('should call zipService with settings', () => {
@@ -303,11 +313,11 @@ describe('#packageService()', () => {
       .then(() => BbPromise.all([
         expect(getExcludesStub).to.be.calledOnce,
         expect(getIncludesStub).to.be.calledOnce,
+        expect(resolveFilePathsFromPatternsStub).to.be.calledOnce,
 
-        expect(zipServiceStub).to.be.calledOnce,
-        expect(zipServiceStub).to.have.been.calledWithExactly(
-          exclude,
-          include,
+        expect(zipFilesStub).to.be.calledOnce,
+        expect(zipFilesStub).to.have.been.calledWithExactly(
+          files,
           zipFileName
         ),
       ]));
@@ -331,7 +341,7 @@ describe('#packageService()', () => {
         .then(() => BbPromise.all([
           expect(getExcludesStub).to.not.have.been.called,
           expect(getIncludesStub).to.not.have.been.called,
-          expect(zipServiceStub).to.not.have.been.called,
+          expect(zipFilesStub).to.not.have.been.called,
         ]));
     });
 
@@ -353,7 +363,7 @@ describe('#packageService()', () => {
         .then(() => BbPromise.all([
           expect(getExcludesStub).to.not.have.been.called,
           expect(getIncludesStub).to.not.have.been.called,
-          expect(zipServiceStub).to.not.have.been.called,
+          expect(zipFilesStub).to.not.have.been.called,
         ]));
     });
   });

lib/plugins/package/lib/zipService.js

@@ -12,6 +12,9 @@ const childProcess = BbPromise.promisifyAll(require('child_process'));
 const globby = require('globby');
 const _ = require('lodash');
 
+const fsStat = BbPromise.promisify(fs.stat);
+const fsReadFile = BbPromise.promisify(fs.readFile);
+
 module.exports = {
   zipService(exclude, include, zipFileName) {
     const params = {
@@ -51,73 +54,58 @@ module.exports = {
   },
 
   zip(params) {
-    const patterns = ['**'];
+    return this.resolveFilePathsFromPatterns(params).then(filePaths =>
+      this.zipFiles(filePaths, params.zipFileName));
+  },
 
-    params.exclude.forEach((pattern) => {
-      if (pattern.charAt(0) !== '!') {
-        patterns.push(`!${pattern}`);
-      } else {
-        patterns.push(pattern.substring(1));
-      }
-    });
-
-    // push the include globs to the end of the array
-    // (files and folders will be re-added again even if they were excluded beforehand)
-    params.include.forEach((pattern) => {
-      patterns.push(pattern);
-    });
+  zipFiles(files, zipFileName) {
+    if (files.length === 0) {
+      const error = new this.serverless.classes.Error('No files to package');
+      return BbPromise.reject(error);
+    }
 
     const zip = archiver.create('zip');
     // Create artifact in temp path and move it to the package path (if any) later
     const artifactFilePath = path.join(this.serverless.config.servicePath,
       '.serverless',
-      params.zipFileName
+      zipFileName
     );
     this.serverless.utils.writeFileDir(artifactFilePath);
 
     const output = fs.createWriteStream(artifactFilePath);
 
-    const files = globby.sync(patterns, {
-      cwd: this.serverless.config.servicePath,
-      dot: true,
-      silent: true,
-      follow: true,
-    });
-
-    if (files.length === 0) {
-      const error = new this.serverless
-        .classes.Error('No file matches include / exclude patterns');
-      return BbPromise.reject(error);
-    }
-
-    output.on('open', () => {
-      zip.pipe(output);
-
-      files.forEach((filePath) => {
-        const fullPath = path.resolve(
-          this.serverless.config.servicePath,
-          filePath
-        );
-
-        const stats = fs.statSync(fullPath);
-
-        if (!stats.isDirectory(fullPath)) {
-          zip.append(fs.readFileSync(fullPath), {
-            name: filePath,
-            mode: stats.mode,
-            date: new Date(0), // necessary to get the same hash when zipping the same content
-          });
-        }
-      });
-
-      zip.finalize();
-    });
-
     return new BbPromise((resolve, reject) => {
       output.on('close', () => resolve(artifactFilePath));
+      output.on('error', (err) => reject(err));
       zip.on('error', (err) => reject(err));
+
+
+      output.on('open', () => {
+        zip.pipe(output);
+
+        BbPromise.all(files.map((filePath) => {
+          const fullPath = path.resolve(
+            this.serverless.config.servicePath,
+            filePath
+          );
+
+          return fsStat(fullPath).then(stats =>
+            this.getFileContent(fullPath).then(fileContent =>
+              zip.append(fileContent, {
+                name: filePath,
+                mode: stats.mode,
+                date: new Date(0), // necessary to get the same hash when zipping the same content
+              })
+            )
+          );
+        })).then(() => zip.finalize()).catch(reject);
+      });
     });
   },
+
+  getFileContent(fullPath) {
+    return fsReadFile(fullPath, 'utf8');
+  },
 };
 
 // eslint-disable-next-line

lib/plugins/package/lib/zipService.test.js

@@ -567,7 +567,7 @@ describe('zipService', () => {
 
         expect(Object.keys(unzippedFileData)
          .filter(file => !unzippedFileData[file].dir))
-         .to.be.lengthOf(13);
+         .to.be.lengthOf(12);
 
         // root directory
         expect(unzippedFileData['event.json'].name)
@@ -648,7 +648,7 @@ describe('zipService', () => {
 
         expect(Object.keys(unzippedFileData)
           .filter(file => !unzippedFileData[file].dir))
-          .to.be.lengthOf(8);
+          .to.be.lengthOf(7);
 
         // root directory
         expect(unzippedFileData['handler.js'].name)
@@ -693,7 +693,7 @@ describe('zipService', () => {
 
         expect(Object.keys(unzippedFileData)
           .filter(file => !unzippedFileData[file].dir))
-          .to.be.lengthOf(11);
+          .to.be.lengthOf(10);
 
         // root directory
         expect(unzippedFileData['event.json'].name)
@@ -747,7 +747,7 @@ describe('zipService', () => {
 
         expect(Object.keys(unzippedFileData)
           .filter(file => !unzippedFileData[file].dir))
-          .to.be.lengthOf(11);
+          .to.be.lengthOf(10);
 
         // root directory
         expect(unzippedFileData['event.json'].name)
@@ -788,4 +788,11 @@ describe('zipService', () => {
         .rejectedWith(Error, 'file matches include / exclude');
     });
   });
+
+  describe('#zipFiles()', () => {
+    it('should throw an error if no files are provided', () =>
+      expect(packagePlugin.zipFiles([], path.resolve(__dirname, 'tmp.zip'))).to.be
+        .rejectedWith(Error, 'No files to package')
+    );
+  });
 });