Basic implementation #1
Conversation
|
Reviewers, please note that I'd like to eventually open source this repo. |
package.json
Outdated
| "aws-sdk": "^2.338.0", | ||
| "fnv-plus": "^1.2.12", | ||
| "jsbn": "^1.1.0", | ||
| "lodash": "^4.17.11" |
There was a problem hiding this comment.
Maybe worth importing the single-function lodash modules that are used? If I'm not mistaken a lighter dependency load could improve lambda cold start time
api/store.js
Outdated
| const { | ||
| bucket, | ||
| debug, | ||
| domain_whitelist: domainWhitelist, |
There was a problem hiding this comment.
With an eye to open-sourcing, you might consider domainSafeList instead
api/store.js
Outdated
| if (!_.find(domainWhitelist, suffixMatcher)) { | ||
| throw new HttpError( | ||
| 400, | ||
| `Not an allowed domain for shortening: ${host}, whitelist: ${domainWhitelist}` |
There was a problem hiding this comment.
It might be undesirable from a security standpoint to surface the entire list of allowable domains to external callers (which I think is what would happen, unless this message is removed from the output later on)
api/store.js
Outdated
| .then(() => hash); | ||
| } | ||
|
|
||
| function sendResponse(statusCode, { message, path }, callback) { |
There was a problem hiding this comment.
My (strong) preference is not to pass callbacks out of scope and allow collaborator functions to invoke them - what do you think of turning this inside out so it just returns the object for the callback, then the main handler function would invoke it like callback(null, buildResponse(statusCode, { message, path })?
api/store.js
Outdated
|
|
||
| do { | ||
| const quotientAndRemainder = workingValue.divideAndRemainder(base); | ||
| const quotient = quotientAndRemainder[0]; |
There was a problem hiding this comment.
can use destructuring to simplify here
const [quotient, remainder] = workingValue.divideAndRemainder(base);
package.json
Outdated
| "engines": { | ||
| "node": "8.10.0" | ||
| }, | ||
| "dependencies": { | ||
| "aws-sdk": "^2.338.0" | ||
| "aws-sdk": "^2.338.0", |
There was a problem hiding this comment.
We don't need to have this as a dependency because AWS Lambda includes this automatically. Including it here is redundant and greatly increases the size of the package that gets uploaded to AWS, which affects cold start time.
If we add it to devDependencies, we can still use it for developing and testing locally.
There was a problem hiding this comment.
Nice! This change reduced the .zip file size from 6.24 MB to 148.2 KB, and it still works!
| describe('When the `url` property', () => { | ||
| test( | ||
| 'is not a valid URL, we get a 400 response', | ||
| () => expectStatusForUrl('not a url', 400) |
There was a problem hiding this comment.
Are we able to add expectations for the error messages as well?
There was a problem hiding this comment.
We were already testing for the existence of a message string in the response body. I've just added a line to additionally test that the message is non-empty... but tests for each individual message string seems like overkill.
|
LGTM ✨ |
| storage and redirection. | ||
|
|
||
| The serverless scaffolding for this was largely lifted from | ||
| [@danielireson](https://github.com/danielireson)'s excellent |
There was a problem hiding this comment.
Wanted to give credit where it's due. :)
|
LGTM! |
https://change.atlassian.net/browse/SHOT-755