Conversation
| // HTTP handlers | ||
|
|
||
| export const getProjects = async (event) => { | ||
| export const getProjects = async (event, ctx, callback) => { |
There was a problem hiding this comment.
Wait i think this is fine, JS params just need to be ordered correctly for how they're called and as long as we have event its fine (since we're not really doing anything with ctx here like ratelimiting or otherwise) but standardization is maybe helpful?
There was a problem hiding this comment.
I would prefer to standardize to avoid any surprises, this is the known syntax across all services, except for btx. Thoughts?
kevinxiao27
left a comment
kevinxiao27
left a comment
There was a problem hiding this comment.
Minor nits
Would just make sure teams API behaviour has not changed (since that .then .catch effects are not translated to a form that has identical behaviour)
| key, | ||
| publicUrl | ||
| }); | ||
| callback?.(null, res); |
There was a problem hiding this comment.
yea this syntax is cursed thank you for removing it
|
|
||
| // WebSocket connect | ||
| export const wsConnect = async (event) => { | ||
| export const wsConnect = async (event, ctx, callback) => { |
There was a problem hiding this comment.
ditto earlier comment, we can decide what we want to do about it though
There was a problem hiding this comment.
same reply as above
|
|
||
| callback(null, response_fail); | ||
| return response_fail; | ||
| const res = await teamHelpers.makeTeam(data.team_name, data.eventID, data.year, data.memberIDs); |
There was a problem hiding this comment.
mmm I'm not sure this is still the same behaviour i'm not sure upon failure the res is null, would probably manually intervene for this edit
There was a problem hiding this comment.
That's a valid point of concern. After looking into it deeper, there is no need to do a check like this, or to have a separate conditional here, since the entire endpoint is wrapped in a try/catch already. Thoughts?
2 participants
#658