From 17531b850325009d6a0250e8f87351a7ccd837e5 Mon Sep 17 00:00:00 2001 From: Gady Piazza Date: Thu, 27 Jul 2017 12:52:46 +0100 Subject: [PATCH 1/8] Do not add .js extension to entries values --- lib/validate.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/validate.js b/lib/validate.js index 2f7b10f9b..2dd689de6 100644 --- a/lib/validate.js +++ b/lib/validate.js @@ -8,7 +8,7 @@ const _ = require('lodash'); function getEntryForFunction(serverlessFunction) { const handler = serverlessFunction.handler; - const handlerFile = /(.*)\..*?$/.exec(handler)[1] + '.js'; + const handlerFile = /(.*)\..*?$/.exec(handler)[1]; // Create a valid entry key return { From c4485260fdbe1e2c81edaee4accca1d668478930 Mon Sep 17 00:00:00 2001 From: Gady Piazza Date: Thu, 27 Jul 2017 15:18:39 +0100 Subject: [PATCH 2/8] Find entries extension from filename --- lib/validate.js | 33 ++++++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/lib/validate.js b/lib/validate.js index 2dd689de6..65f88eb80 100644 --- a/lib/validate.js +++ b/lib/validate.js @@ -3,16 +3,38 @@ const BbPromise = require('bluebird'); const path = require('path'); const fse = require('fs-extra'); +const fs = require('fs'); const lib = require('./index'); const _ = require('lodash'); -function getEntryForFunction(serverlessFunction) { +function listFiles(dir, filelist) { + const files = fs.readdirSync(dir); + filelist = filelist || []; + files.forEach(function(file) { + if (fs.statSync(path.join(dir, file)).isDirectory()) { + filelist = listFiles(path.join(dir, file), filelist); + } + else { + filelist.push(file); + } + }); + return filelist; +} + +function getEntryExtension(fileName, files) { + const fileFullName = files.find(file => file.startsWith(fileName)); + return path.extname(fileFullName); +} + +function getEntryForFunction(serverlessFunction, files) { const handler = serverlessFunction.handler; - const handlerFile = /(.*)\..*?$/.exec(handler)[1]; + const handlerFile = /(.*)\..*?$/.exec(handler)[1]; + + const ext = getEntryExtension(handlerFile, files); // Create a valid entry key return { - [handlerFile]: `./${handlerFile}` + [handlerFile]: `./${handlerFile}${ext}` }; }; @@ -27,13 +49,14 @@ module.exports = { // Expose entries - must be done before requiring the webpack configuration const entries = {}; const functions = this.serverless.service.getAllFunctions(); + const allFilesInServicePath = listFiles(this.serverless.config.servicePath); if (this.options.function) { const serverlessFunction = this.serverless.service.getFunction(this.options.function); - const entry = getEntryForFunction(serverlessFunction); + const entry = getEntryForFunction(serverlessFunction, allFilesInServicePath); _.merge(entries, entry); } else { _.forEach(functions, func => { - const entry = getEntryForFunction(this.serverless.service.getFunction(func)); + const entry = getEntryForFunction(this.serverless.service.getFunction(func), allFilesInServicePath); _.merge(entries, entry); }); } From 5a6bbc54f7a490c07de37cb94e12002bfabd18d4 Mon Sep 17 00:00:00 2001 From: Gady Piazza Date: Thu, 27 Jul 2017 15:47:25 +0100 Subject: [PATCH 3/8] Better filesearch performance --- lib/validate.js | 31 ++++++++----------------------- package.json | 1 + 2 files changed, 9 insertions(+), 23 deletions(-) diff --git a/lib/validate.js b/lib/validate.js index 65f88eb80..befe8783d 100644 --- a/lib/validate.js +++ b/lib/validate.js @@ -3,34 +3,20 @@ const BbPromise = require('bluebird'); const path = require('path'); const fse = require('fs-extra'); -const fs = require('fs'); +const glob = require('glob'); const lib = require('./index'); const _ = require('lodash'); -function listFiles(dir, filelist) { - const files = fs.readdirSync(dir); - filelist = filelist || []; - files.forEach(function(file) { - if (fs.statSync(path.join(dir, file)).isDirectory()) { - filelist = listFiles(path.join(dir, file), filelist); - } - else { - filelist.push(file); - } - }); - return filelist; -} - -function getEntryExtension(fileName, files) { - const fileFullName = files.find(file => file.startsWith(fileName)); - return path.extname(fileFullName); +function getEntryExtension(fileName, servicePath) { + let files = glob.sync(`${fileName}.*`, { cwd: servicePath }); + return path.extname(files[0]); } -function getEntryForFunction(serverlessFunction, files) { +function getEntryForFunction(serverlessFunction, servicePath) { const handler = serverlessFunction.handler; const handlerFile = /(.*)\..*?$/.exec(handler)[1]; - const ext = getEntryExtension(handlerFile, files); + const ext = getEntryExtension(handlerFile, servicePath); // Create a valid entry key return { @@ -49,14 +35,13 @@ module.exports = { // Expose entries - must be done before requiring the webpack configuration const entries = {}; const functions = this.serverless.service.getAllFunctions(); - const allFilesInServicePath = listFiles(this.serverless.config.servicePath); if (this.options.function) { const serverlessFunction = this.serverless.service.getFunction(this.options.function); - const entry = getEntryForFunction(serverlessFunction, allFilesInServicePath); + const entry = getEntryForFunction(serverlessFunction, this.serverless.config.servicePath); _.merge(entries, entry); } else { _.forEach(functions, func => { - const entry = getEntryForFunction(this.serverless.service.getFunction(func), allFilesInServicePath); + const entry = getEntryForFunction(this.serverless.service.getFunction(func), this.serverless.config.servicePath); _.merge(entries, entry); }); } diff --git a/package.json b/package.json index 522db5575..8ad86932a 100644 --- a/package.json +++ b/package.json @@ -31,6 +31,7 @@ "body-parser": "^1.15.2", "express": "^4.14.0", "fs-extra": "^0.26.7", + "glob": "^7.1.2", "lodash": "^4.17.4", "npm-programmatic": "0.0.5", "ts-node": "^3.2.0" From 0e432a487dc8aa29c27b52fb55e72317ae0d0ccd Mon Sep 17 00:00:00 2001 From: Gady Piazza Date: Thu, 27 Jul 2017 16:14:44 +0100 Subject: [PATCH 4/8] Add warning for multiple files found --- lib/validate.js | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/lib/validate.js b/lib/validate.js index befe8783d..70b589850 100644 --- a/lib/validate.js +++ b/lib/validate.js @@ -7,25 +7,29 @@ const glob = require('glob'); const lib = require('./index'); const _ = require('lodash'); -function getEntryExtension(fileName, servicePath) { - let files = glob.sync(`${fileName}.*`, { cwd: servicePath }); - return path.extname(files[0]); -} +module.exports = { + validate() { + const getEntryExtension = (fileName, servicePath) => { + const files = glob.sync(`${fileName}.*`, { cwd: servicePath }); -function getEntryForFunction(serverlessFunction, servicePath) { - const handler = serverlessFunction.handler; - const handlerFile = /(.*)\..*?$/.exec(handler)[1]; - - const ext = getEntryExtension(handlerFile, servicePath); + if (_.size(files) > 1) { + this.serverless.cli.log(`WARNING: More than one matching handlers found for ${fileName}. Using ${_.first(files)}.`); + } + return path.extname(_.first(files)); + } - // Create a valid entry key - return { - [handlerFile]: `./${handlerFile}${ext}` - }; -}; + const getEntryForFunction = (serverlessFunction, servicePath) => { + const handler = serverlessFunction.handler; + const handlerFile = /(.*)\..*?$/.exec(handler)[1]; + + const ext = getEntryExtension(handlerFile, servicePath); + + // Create a valid entry key + return { + [handlerFile]: `./${handlerFile}${ext}` + }; + }; -module.exports = { - validate() { this.webpackConfig = ( this.serverless.service.custom && this.serverless.service.custom.webpack || From 97d513d1ca9835a68b5c6d8489a76f6a9cad2fb2 Mon Sep 17 00:00:00 2001 From: Frank Schmid Date: Thu, 27 Jul 2017 21:20:58 +0200 Subject: [PATCH 5/8] Do not match directories --- lib/validate.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/validate.js b/lib/validate.js index 70b589850..63c5cd9ac 100644 --- a/lib/validate.js +++ b/lib/validate.js @@ -10,7 +10,7 @@ const _ = require('lodash'); module.exports = { validate() { const getEntryExtension = (fileName, servicePath) => { - const files = glob.sync(`${fileName}.*`, { cwd: servicePath }); + const files = glob.sync(`${fileName}.*`, { cwd: servicePath, nodir: true }); if (_.size(files) > 1) { this.serverless.cli.log(`WARNING: More than one matching handlers found for ${fileName}. Using ${_.first(files)}.`); @@ -21,7 +21,7 @@ module.exports = { const getEntryForFunction = (serverlessFunction, servicePath) => { const handler = serverlessFunction.handler; const handlerFile = /(.*)\..*?$/.exec(handler)[1]; - + const ext = getEntryExtension(handlerFile, servicePath); // Create a valid entry key From e48ed16493b13059367d079a1d6738cb601b6664 Mon Sep 17 00:00:00 2001 From: Frank Schmid Date: Thu, 27 Jul 2017 22:26:28 +0200 Subject: [PATCH 6/8] Added unit tests. Throw error if handler is not found. --- lib/validate.js | 23 +++++--- tests/validate.test.js | 124 ++++++++++++++++++++++++++++++++--------- 2 files changed, 114 insertions(+), 33 deletions(-) diff --git a/lib/validate.js b/lib/validate.js index 63c5cd9ac..dba799b63 100644 --- a/lib/validate.js +++ b/lib/validate.js @@ -9,20 +9,27 @@ const _ = require('lodash'); module.exports = { validate() { - const getEntryExtension = (fileName, servicePath) => { - const files = glob.sync(`${fileName}.*`, { cwd: servicePath, nodir: true }); + const getEntryExtension = fileName => { + const files = glob.sync(`${fileName}.*`, { + cwd: this.serverless.config.servicePath, + nodir: true + }); + if (_.isEmpty(files)) { + // If we cannot find any handler we should terminate with an error + throw new this.serverless.classes.Error(`No matching handler found for '${fileName}'. Check your service definition.`); + } if (_.size(files) > 1) { - this.serverless.cli.log(`WARNING: More than one matching handlers found for ${fileName}. Using ${_.first(files)}.`); + this.serverless.cli.log(`WARNING: More than one matching handlers found for '${fileName}'. Using '${_.first(files)}'.`); } return path.extname(_.first(files)); } - const getEntryForFunction = (serverlessFunction, servicePath) => { + const getEntryForFunction = serverlessFunction => { const handler = serverlessFunction.handler; const handlerFile = /(.*)\..*?$/.exec(handler)[1]; - const ext = getEntryExtension(handlerFile, servicePath); + const ext = getEntryExtension(handlerFile); // Create a valid entry key return { @@ -41,16 +48,16 @@ module.exports = { const functions = this.serverless.service.getAllFunctions(); if (this.options.function) { const serverlessFunction = this.serverless.service.getFunction(this.options.function); - const entry = getEntryForFunction(serverlessFunction, this.serverless.config.servicePath); + const entry = getEntryForFunction(serverlessFunction); _.merge(entries, entry); } else { _.forEach(functions, func => { - const entry = getEntryForFunction(this.serverless.service.getFunction(func), this.serverless.config.servicePath); + const entry = getEntryForFunction(this.serverless.service.getFunction(func)); _.merge(entries, entry); }); } lib.entries = entries; - + // Expose service file and options lib.serverless = this.serverless; lib.options = this.options; diff --git a/tests/validate.test.js b/tests/validate.test.js index 584cda6b1..8886da7f5 100644 --- a/tests/validate.test.js +++ b/tests/validate.test.js @@ -10,16 +10,24 @@ const makeFsExtraMock = require('./fs-extra.mock'); chai.use(require('sinon-chai')); const expect = chai.expect; +const globMock = { + sync() {} +}; + describe('validate', () => { let fsExtraMock; let baseModule; let module; let serverless; + let sandbox; before(() => { + sandbox = sinon.sandbox.create(); + mockery.enable({ warnOnUnregistered: false }); fsExtraMock = makeFsExtraMock(); mockery.registerMock('fs-extra', fsExtraMock); + mockery.registerMock('glob', globMock); baseModule = require('../lib/validate'); Object.freeze(baseModule); }); @@ -31,6 +39,9 @@ describe('validate', () => { beforeEach(() => { serverless = new Serverless(); + serverless.cli = { + log: sandbox.stub() + }; fsExtraMock._resetSpies(); module = Object.assign({ serverless, @@ -38,6 +49,10 @@ describe('validate', () => { }, baseModule); }); + afterEach(() => { + sandbox.restore(); + }); + it('should expose a `validate` method', () => { expect(module.validate).to.be.a('function'); }); @@ -266,6 +281,12 @@ describe('validate', () => { }); describe('entries', () => { + let globSyncStub; + + beforeEach(() => { + globSyncStub = sandbox.stub(globMock, 'sync'); + }); + const testFunctionsConfig = { func1: { handler: 'module1.func1handler', @@ -305,7 +326,7 @@ describe('validate', () => { }, }; - it('should expose entries from serverless.yml if `options.function` is not defined', () => { + it('should expose all functions if `options.function` is not defined', () => { const testOutPath = 'test'; const testConfig = { entry: 'test', @@ -316,22 +337,24 @@ describe('validate', () => { }; module.serverless.service.custom.webpack = testConfig; module.serverless.service.functions = testFunctionsConfig; - return module - .validate() - .then(() => { - const lib = require('../lib/index'); - const expectedLibEntries = { - 'module1.js': './module1.js', - 'module2.js': './module2.js', - 'handlers/func3/module2.js': './handlers/func3/module2.js', - 'handlers/module2/func3/module2.js': './handlers/module2/func3/module2.js', - }; - - expect(lib.entries).to.deep.eq(expectedLibEntries) - }); + globSyncStub.callsFake(filename => [ _.replace(filename, '*', 'js') ]); + return expect(module.validate()).to.be.fulfilled + .then(() => { + const lib = require('../lib/index'); + const expectedLibEntries = { + 'module1': './module1.js', + 'module2': './module2.js', + 'handlers/func3/module2': './handlers/func3/module2.js', + 'handlers/module2/func3/module2': './handlers/module2/func3/module2.js', + }; + + expect(lib.entries).to.deep.equal(expectedLibEntries); + expect(globSyncStub).to.have.callCount(4); + expect(serverless.cli.log).to.not.have.been.called; + }); }); - it('should expose entries with `options.function` value if `options.function` is defined and found in entries from serverless.yml', () => { + it('should expose the requested function if `options.function` is defined and the function is found', () => { const testOutPath = 'test'; const testFunction = 'func1'; const testConfig = { @@ -344,16 +367,67 @@ describe('validate', () => { module.serverless.service.custom.webpack = testConfig; module.serverless.service.functions = testFunctionsConfig; module.options.function = testFunction; - return module - .validate() - .then(() => { - const lib = require('../lib/index'); - const expectedLibEntries = { - 'module1.js': './module1.js' - }; - - expect(lib.entries).to.deep.eq(expectedLibEntries) - }); + globSyncStub.callsFake(filename => [ _.replace(filename, '*', 'js') ]); + return expect(module.validate()).to.be.fulfilled + .then(() => { + const lib = require('../lib/index'); + const expectedLibEntries = { + 'module1': './module1.js' + }; + + expect(lib.entries).to.deep.equal(expectedLibEntries) + expect(globSyncStub).to.have.been.calledOnce; + expect(serverless.cli.log).to.not.have.been.called; + }); + }); + + it('should show a warning, if more than one matching handler is found', () => { + const testOutPath = 'test'; + const testFunction = 'func1'; + const testConfig = { + entry: 'test', + context: 'testcontext', + output: { + path: testOutPath, + }, + }; + module.serverless.service.custom.webpack = testConfig; + module.serverless.service.functions = testFunctionsConfig; + module.options.function = testFunction; + globSyncStub.returns([ 'module1.ts', 'module1.js' ]); + return expect(module.validate()).to.be.fulfilled + .then(() => { + const lib = require('../lib/index'); + const expectedLibEntries = { + 'module1': './module1.ts' + }; + + expect(lib.entries).to.deep.equal(expectedLibEntries) + expect(globSyncStub).to.have.been.calledOnce; + expect(serverless.cli.log).to.have.been.calledOnce; + expect(serverless.cli.log).to.have.been.calledWith( + 'WARNING: More than one matching handlers found for \'module1\'. Using \'module1.ts\'.' + ); + }); + }); + + it('should throw an exception if no handler is found', () => { + const testOutPath = 'test'; + const testFunction = 'func1'; + const testConfig = { + entry: 'test', + context: 'testcontext', + output: { + path: testOutPath, + }, + }; + module.serverless.service.custom.webpack = testConfig; + module.serverless.service.functions = testFunctionsConfig; + module.options.function = testFunction; + globSyncStub.returns([]); + expect(() => { + module.validate(); + }).to.throw(/No matching handler found for/); }); it('should throw an exception if `options.function` is defined but not found in entries from serverless.yml', () => { From 4556653ccb3e6d2f74ee792a72aed5ab96ee10ce Mon Sep 17 00:00:00 2001 From: Frank Schmid Date: Fri, 28 Jul 2017 10:19:36 +0200 Subject: [PATCH 7/8] Fixed examples --- examples/babel-dynamically-entries/webpack.config.js | 2 +- examples/babel-multiple-statically-entries/webpack.config.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/examples/babel-dynamically-entries/webpack.config.js b/examples/babel-dynamically-entries/webpack.config.js index 293bd3d6c..3c723449a 100644 --- a/examples/babel-dynamically-entries/webpack.config.js +++ b/examples/babel-dynamically-entries/webpack.config.js @@ -15,6 +15,6 @@ module.exports = { output: { libraryTarget: 'commonjs', path: path.join(__dirname, '.webpack'), - filename: '[name]' + filename: '[name].js' } }; diff --git a/examples/babel-multiple-statically-entries/webpack.config.js b/examples/babel-multiple-statically-entries/webpack.config.js index 76f2520e4..3ff5b0403 100644 --- a/examples/babel-multiple-statically-entries/webpack.config.js +++ b/examples/babel-multiple-statically-entries/webpack.config.js @@ -17,6 +17,6 @@ module.exports = { output: { libraryTarget: 'commonjs', path: path.join(__dirname, '.webpack'), - filename: '[name]' + filename: '[name].js' } }; From ffe51a255248f10911f81de7ac5b945861d465c5 Mon Sep 17 00:00:00 2001 From: Frank Schmid Date: Fri, 28 Jul 2017 11:29:01 +0200 Subject: [PATCH 8/8] Use preference sorting to find the most probable handler for multiple hits --- lib/validate.js | 29 ++++++++++++++++++++++++++--- tests/validate.test.js | 32 +++++++++++++++++++++++++++++++- 2 files changed, 57 insertions(+), 4 deletions(-) diff --git a/lib/validate.js b/lib/validate.js index dba799b63..c87526663 100644 --- a/lib/validate.js +++ b/lib/validate.js @@ -7,6 +7,17 @@ const glob = require('glob'); const lib = require('./index'); const _ = require('lodash'); +/** + * For automatic entry detection we sort the found files to solve ambiguities. + * This should cover most of the cases. For complex setups the user should + * build his own entries with help of the other exports. + */ +const preferredExtensions = [ + '.js', + '.ts', + '.jsx' +]; + module.exports = { validate() { const getEntryExtension = fileName => { @@ -19,10 +30,22 @@ module.exports = { // If we cannot find any handler we should terminate with an error throw new this.serverless.classes.Error(`No matching handler found for '${fileName}'. Check your service definition.`); } - if (_.size(files) > 1) { - this.serverless.cli.log(`WARNING: More than one matching handlers found for '${fileName}'. Using '${_.first(files)}'.`); + + // Move preferred file extensions to the beginning + const sortedFiles = _.uniq( + _.concat( + _.sortBy( + _.filter(files, file => _.includes(preferredExtensions, path.extname(file))), + a => _.size(a) + ), + files + ) + ); + + if (_.size(sortedFiles) > 1) { + this.serverless.cli.log(`WARNING: More than one matching handlers found for '${fileName}'. Using '${_.first(sortedFiles)}'.`); } - return path.extname(_.first(files)); + return path.extname(_.first(sortedFiles)); } const getEntryForFunction = serverlessFunction => { diff --git a/tests/validate.test.js b/tests/validate.test.js index 8886da7f5..62555c6fc 100644 --- a/tests/validate.test.js +++ b/tests/validate.test.js @@ -381,7 +381,7 @@ describe('validate', () => { }); }); - it('should show a warning, if more than one matching handler is found', () => { + it('should show a warning if more than one matching handler is found', () => { const testOutPath = 'test'; const testFunction = 'func1'; const testConfig = { @@ -411,6 +411,36 @@ describe('validate', () => { }); }); + it('should select the most probable handler if multiple hits are found', () => { + const testOutPath = 'test'; + const testFunction = 'func1'; + const testConfig = { + entry: 'test', + context: 'testcontext', + output: { + path: testOutPath, + }, + }; + module.serverless.service.custom.webpack = testConfig; + module.serverless.service.functions = testFunctionsConfig; + module.options.function = testFunction; + globSyncStub.returns([ 'module1.doc', 'module1.json', 'module1.test.js', 'module1.ts', 'module1.js' ]); + return expect(module.validate()).to.be.fulfilled + .then(() => { + const lib = require('../lib/index'); + const expectedLibEntries = { + 'module1': './module1.ts' + }; + + expect(lib.entries).to.deep.equal(expectedLibEntries) + expect(globSyncStub).to.have.been.calledOnce; + expect(serverless.cli.log).to.have.been.calledOnce; + expect(serverless.cli.log).to.have.been.calledWith( + 'WARNING: More than one matching handlers found for \'module1\'. Using \'module1.ts\'.' + ); + }); + }); + it('should throw an exception if no handler is found', () => { const testOutPath = 'test'; const testFunction = 'func1';