Do not add .js extension to entries values #167

kikar · 2017-07-27T11:54:04Z

What did you implement:

Closes #165

How did you implement it:

How can we verify it:

Todos:

Write tests
Write documentation
Fix linting errors (not yet implemented)
Make sure code coverage hasn't dropped (not yet implemented)
Provide verification config / commands / resources
Enable "Allow edits from maintainers" for this PR
Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

HyperBrain · 2017-07-27T12:52:45Z

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];


We should actually retrieve the correct file extension here and set it in the value of the key, so that there's no need to use custom code in the webpack configuration to get the plugin running.

function getHandlerFileExtension(baseName) { // Use fs/fs-extra/path or any other appropriate mechanism to determine the actual file extension of the handler // Maybe we can search for known and supported endings in a predefined order // So that the standard extension 'js' is preferred if, by accident, there are multiple copies // of the handler with different extensions - or throw an error in that case. const allowedExtensions = [ 'js', 'ts', 'jsx' ]; const handlerExtension = ...; ... return handlerExtension; } ... const handlerFileBaseName = /(.*)\..*?$/.exec(handler)[1]; const handerFile = `${handlerFileBaseName}.${getHandlerFileExtension(handlerFileBaseName)}`; ... return { [handlerFileBaseName]: `./${handlerFile}` };

Do we assume all the function files are in the servicePath folder?

At least somewhere under the servicePath. The handlerBaseName contains the path elements relative to the handler. So, for example, it could be handlers/shop/order/handler and handlers/shop/fulfill/handler, but we can assume that handler files are never outside the service folder (of course the user can use symlinks to access files outside the service directory, but checking if the baseName + extension exists, will automatically reveil the correct results in this case)

Better rename the handlerFileBaseName variable to handlerFilePathWithBaseName as it actually contains the path + the basename.

HyperBrain · 2017-07-27T12:54:03Z

tests/validate.test.js

-            'module2.js': './module2.js',
-            'handlers/func3/module2.js': './handlers/func3/module2.js',
-            'handlers/module2/func3/module2.js': './handlers/module2/func3/module2.js',
+            'module1': './module1',


The tests have to be adapted AFTER the changes above, because the used external functions to determine the extension have to be stubbed with sinon.

HyperBrain · 2017-07-27T13:08:59Z

Determining the extension will also support the case where single functions in a project are updated and the used extension changes. E.g. if you change a function from js to jsx or from js to ts. Then the next deploy will just work out of the box because your config file does not need to know anything about the specific function configurations.

kikar · 2017-07-27T14:19:29Z

Latest commit should do the job. Problem is that all tests fail cause we don't actually have files to check

HyperBrain · 2017-07-27T14:27:42Z

lib/validate.js

@@ -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);


Is it necessary to browse the whole service dir file tree here? I think that is very expensive - the directory might include node_modules etc.

Assuming that the handlers we have already point to the handler files we check, we know exactly where the files have to be. So just searching in getEntryExtension(handlerFile) for ${handlerFile}.* or ${handlerFile}.{js,ts,jsx} should retrieve the full file automatically (and even shows us if it is ambiguous, which is an error on the user side if multiple of our allowed extensions are found). We do not need to be aware of the complete file hierarchy here.

HyperBrain · 2017-07-27T14:33:35Z

I can take care of the unit tests as soon as we have a performant and working solution.

kikar · 2017-07-27T14:48:07Z

Improved the file search as recommended

HyperBrain · 2017-07-27T14:52:13Z

lib/validate.js

 const lib = require('./index');
 const _ = require('lodash');

-function getEntryForFunction(serverlessFunction) {
+function getEntryExtension(fileName, servicePath) {
+  let files = glob.sync(`${fileName}.*`, { cwd: servicePath });


can be const

HyperBrain · 2017-07-27T14:55:58Z

lib/validate.js

-function getEntryForFunction(serverlessFunction) {
+function getEntryExtension(fileName, servicePath) {
+  let files = glob.sync(`${fileName}.*`, { cwd: servicePath });
+  return path.extname(files[0]);


We could emit a warning in case more than one file (or no file at all) is found here:

if (_.size(files) > 1) { this.serverless.cli.log(`WARNING: More than one matching handlers found for ${fileName}. Using ${_.first(files)}.`); }

Of course the now global function(s) have to be moved into validate() to have access to the correct this.

Also use _.first(files) here instead of files[0] as the lodash variant shows our intention in a better way and is safer.

HyperBrain · 2017-07-27T17:30:07Z

I will add the unit tests and the doc/example updates tomorrow, so that we can release 2.2.0 also tomorrow.

HyperBrain · 2017-07-27T20:33:36Z

I tested it with one of our projects. That worked nicely. Also rebased onto the current webpack master.
Everything should be ok now. The only thing that can happen is, that if there are unrelated files besides the handler, that just have a different ending, the glob could match them (e.g. handler.doc). However it will show the warning we implemented.

If that turns out to be a problem later, we can restrict the allowed extensions by setting the glob pattern to ${fileName}.@(js|ts|jsx).

… hits

HyperBrain · 2017-07-28T09:31:09Z

Just added some more complex test cases and a preference sorting. I will do some final tests now.
For most standard uses of lib.entries it should work now - if someone needs more sophisticated handling he can program the complete stuff in his webpack.conf - thanks to the serverless and option exports.

HyperBrain mentioned this pull request Jul 27, 2017

Expose service and options for dynamicity #168

Merged

2 tasks

HyperBrain requested changes Jul 27, 2017

View reviewed changes

HyperBrain mentioned this pull request Jul 27, 2017

Add serverless create template #137

Closed

HyperBrain reviewed Jul 27, 2017

View reviewed changes

HyperBrain requested changes Jul 27, 2017

View reviewed changes

kikar and others added 6 commits July 27, 2017 21:30

Do not add .js extension to entries values

17531b8

Find entries extension from filename

c448526

Better filesearch performance

5a6bbc5

Add warning for multiple files found

0e432a4

Do not match directories

97d513d

Added unit tests. Throw error if handler is not found.

e48ed16

HyperBrain approved these changes Jul 27, 2017

View reviewed changes

Frank Schmid added 2 commits July 28, 2017 10:19

Fixed examples

4556653

Use preference sorting to find the most probable handler for multiple…

ffe51a2

… hits

HyperBrain merged commit e7531dc into serverless-heaven:master Jul 28, 2017

jerep6 mentioned this pull request Aug 27, 2017

"No matching handler found" when handlers aren't in the root folder #207

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not add .js extension to entries values #167

Do not add .js extension to entries values #167

kikar commented Jul 27, 2017 •

edited by HyperBrain

Loading

HyperBrain Jul 27, 2017

kikar Jul 27, 2017

HyperBrain Jul 27, 2017 •

edited

Loading

HyperBrain Jul 27, 2017

HyperBrain Jul 27, 2017

HyperBrain commented Jul 27, 2017 •

edited

Loading

kikar commented Jul 27, 2017

HyperBrain Jul 27, 2017 •

edited

Loading

HyperBrain commented Jul 27, 2017

kikar commented Jul 27, 2017

HyperBrain Jul 27, 2017

HyperBrain Jul 27, 2017 •

edited

Loading

HyperBrain commented Jul 27, 2017

HyperBrain commented Jul 27, 2017 •

edited

Loading

HyperBrain commented Jul 28, 2017

Do not add .js extension to entries values #167

Do not add .js extension to entries values #167

Conversation

kikar commented Jul 27, 2017 • edited by HyperBrain Loading

What did you implement:

How did you implement it:

How can we verify it:

Todos:

HyperBrain Jul 27, 2017

Choose a reason for hiding this comment

kikar Jul 27, 2017

Choose a reason for hiding this comment

HyperBrain Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

HyperBrain Jul 27, 2017

Choose a reason for hiding this comment

HyperBrain Jul 27, 2017

Choose a reason for hiding this comment

HyperBrain commented Jul 27, 2017 • edited Loading

kikar commented Jul 27, 2017

HyperBrain Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

HyperBrain commented Jul 27, 2017

kikar commented Jul 27, 2017

HyperBrain Jul 27, 2017

Choose a reason for hiding this comment

HyperBrain Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

HyperBrain commented Jul 27, 2017

HyperBrain commented Jul 27, 2017 • edited Loading

HyperBrain commented Jul 28, 2017

kikar commented Jul 27, 2017 •

edited by HyperBrain

Loading

HyperBrain Jul 27, 2017 •

edited

Loading

HyperBrain commented Jul 27, 2017 •

edited

Loading

HyperBrain Jul 27, 2017 •

edited

Loading

HyperBrain Jul 27, 2017 •

edited

Loading

HyperBrain commented Jul 27, 2017 •

edited

Loading