Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

Augment LambdaServer to use URL path parameter from request to lookup #366

Merged
merged 2 commits into from Aug 24, 2017
Merged

Augment LambdaServer to use URL path parameter from request to lookup #366

merged 2 commits into from Aug 24, 2017

Conversation

breathe
Copy link
Contributor

@breathe breathe commented Aug 23, 2017

module information when path information is supplied in url.

Towards closing #334

Not specifically mentioned in the discussion on the issue -- was the question of how to encode the module and handler names in the url. In this PR I interpret the ":" character as a special character to separate module path from handler -- I believe ':''s are not allowed in node paths ...?

That may not be the 'right way' though -- serverless config file uses a '.' format which I personally find to be a pretty bad choice -- maybe using that convention would be better here ... example code in use in various bst context's uses path/to/SomeFunction.js as the module representation though and I was worried about compatibility if I tried to use '.' as the path/handler separator ...

@coveralls
Copy link

coveralls commented Aug 23, 2017

Coverage Status

Coverage decreased (-0.6%) to 86.032% when pulling c66c789 on breathe:master into 6afb5e0 on bespoken:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.6%) to 86.032% when pulling c66c789 on breathe:master into 6afb5e0 on bespoken:master.

@jkelvie
Copy link
Member

jkelvie commented Aug 23, 2017

It's a good question on the separator - I believe the dot is the standard for Lambdas (at least Node.js ones):
http://docs.aws.amazon.com/lambda/latest/dg/API_CreateFunction.html#API_CreateFunction_RequestSyntax
Take a look at the "Handler" property, in particular this:
screen shot 2017-08-22 at 11 31 55 pm

Otherwise, looking great.

And FYI, we changed around the build process to make it easier to handle external pull requests. Now all PRs are run with Travis/Coveralls without any environment variables.

Once pulled in, they are run in Circle with full environment variables and more complete test coverage.

@breathe
Copy link
Contributor Author

breathe commented Aug 23, 2017

I'll change it to use the '.' and also update the validation to error if more than one . is present in the path (aws will fail to execute a lambda with more than one '.' in its path so we might as well be compatible with that behavior here too)

@@ -107,7 +121,7 @@ export class LambdaServer {
console.log(JSON.stringify(bodyJSON, null, 2));
}

const handlerFunction = this.functionName ? this.functionName : "handler";
handlerFunction = handlerFunction != null ? handlerFunction : this.functionName ? this.functionName : "handler";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend going with
handlerFunction = handlerFunction ? handlerFunction : ...
To include undefined apart from null, right now it's undefined if request.url is "/"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(undefined != null) is false though thanks to single '=' in javascript's single != comparison operator ... so I believe the above should work as desired when handlerFunction is either null or undefined.

I changed it though as the your version is prettier and works too (except when supplied 'falsey' values which doesn't matter here).

Copy link
Contributor

@jperata jperata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor change

module information when path information is supplied in url
…er than ':'

Change spelling of handlerFunction comparison logic
@coveralls
Copy link

coveralls commented Aug 23, 2017

Coverage Status

Coverage decreased (-0.5%) to 86.186% when pulling ec9de33 on breathe:master into 2c1612f on bespoken:master.

@jperata jperata merged commit 18b4eee into bespoken:master Aug 24, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants