docs: Added tutorial #1065
docs: Added tutorial #1065
Conversation
|
This PR shows 2 files changed. |
docs/src/tutorial.rst
Outdated
| try { | ||
| var method = event.httpMethod; | ||
|
|
||
| if (method == "GET") { |
docs/src/tutorial.rst
Outdated
| var method = event.httpMethod; | ||
|
|
||
| if (method == "GET") { | ||
| if (event.path.length == 1) { |
docs/src/tutorial.rst
Outdated
| } | ||
| } catch(error) { | ||
| var body = error.stack || JSON.stringify(error, null, 2); | ||
| var response = makeError(body); |
There was a problem hiding this comment.
Again, where are you showing me what code to copy/paste to obtain this makeError function?
The code will not be complete without this.
There was a problem hiding this comment.
I replaced the catch return with:
return {
statusCode: 200,
headers: {},
body: body
};
There was a problem hiding this comment.
That ties into another issue that I raised: how will people know it's an error response?
Either set statusCode: 500 or make body a JSON object with { success: false, error: ... }.
If you do make body a JSON object, I suspect you also need to set a content-type header.
docs/src/tutorial.rst
Outdated
| // in the source file widgets(.js) in the resources directory | ||
| // to handle requests through API Gateway | ||
| const handler = new lambda.Function(this, 'WidgetHandler', { | ||
| runtime: lambda.Runtime.NodeJS610, |
There was a problem hiding this comment.
You switched your code to using async/await (better!) but you also have to update the runtime to use a NodeJS version of 8 or higher, otherwise it's not going to work.
Is the code tested?
There was a problem hiding this comment.
I plan to replace the code in the file with literalinclude directives so I can test the code.
Currently the code is in separate files as suggested by the doc.
docs/src/tutorial.rst
Outdated
| try { | ||
| var method = event.httpMethod; | ||
|
|
||
| if (method == "GET") { |
docs/src/tutorial.rst
Outdated
| else if (method == "DELETE") { | ||
| // DELETE /name | ||
| // Make sure we have a name | ||
| if (event.path.length == 1) { |
docs/src/tutorial.rst
Outdated
| var body = "NAME arg missing (/ supplied, not /name)"; | ||
|
|
||
| return { | ||
| statusCode: 200, |
docs/src/tutorial.rst
Outdated
| return { | ||
| statusCode: 200, | ||
| headers: {}, | ||
| body: JSON.stringify(body) |
There was a problem hiding this comment.
Don't need to stringify body here, as we KNOW it's a string. Just write:
return {
statusCode: 400,
body: 'Widget name missing'
};There was a problem hiding this comment.
I've fixed all of the returns.
docs/src/tutorial.rst
Outdated
| body: JSON.stringify(body) | ||
| }; | ||
| } | ||
| else { |
There was a problem hiding this comment.
You just returned in the previous branch, so no need to put this in an else.
docs/src/tutorial.rst
Outdated
| Bucket: bucketName, Key: name | ||
| }).promise(); | ||
|
|
||
| var body = { success: true } |
There was a problem hiding this comment.
So in case of success, you're returning an object with { success: true } in it, but in case of failure you're returning a string?
Such an irregular API shape is a pain to work with. Better to return the following then in case of error:
{
success: false,
error: 'This is my error message'
}There was a problem hiding this comment.
Good idea, I'll make the change(s).
| } catch(error) { | ||
| var body = error.stack || JSON.stringify(error, null, 2); | ||
| return { | ||
| statusCode: 400, |
There was a problem hiding this comment.
Technically speaking this should be a 500.
4xx = you did something wrong
5xx = I did something wrong
(from the point of view of the server).
In this case we don't know what went wrong, so we can't say for sure it's the client that messed up. In fact, because we didn't predict and handle error beforehand, it is "us" (the developers) that did something wrong.
But it's not super important either way, so no need to change it.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.
More tutorial changes based on Rico's feedback (see PR #1047).