change the way response is used
#55
Conversation
bitgopatmcl
force-pushed
the
response-in-express-wrapper
branch
2 times, most recently
from
30b74f7 to
29be96e
Compare
bitgopatmcl
changed the title
response is used
|
|
||
| export type RouteHandler<R extends HttpRoute> = | ||
| | ServiceFunction<R> | ||
| | { middleware: express.RequestHandler[]; function: ServiceFunction<R> }; |
There was a problem hiding this comment.
This change ended up in here from when I was attempting larger overhauls of the middleware system. I can revert this but would at least like to keep the ability to specify a bare service function without using a single element array.
There was a problem hiding this comment.
This seems like a good usability win, but oh boy do I hate irregularities and added complexity 🙈
We'll probably leave this feat in but def don't want to become the kitchen sink
| } | ||
|
|
||
| const isHttpVerb = (verb: string): verb is 'get' | 'put' | 'post' | 'delete' => | ||
| ({ get: 1, put: 1, post: 1, delete: 1 }.hasOwnProperty(verb)); |
There was a problem hiding this comment.
hasOwnProperty seems the scenic route here, in this low-level library I've been focused on speed, simplicity, and reliability.
Whereas this implementation involves function calls, standard library implementations, iterating, garbage collection, can we use an approach that can short-circuit and use less memory? Something like
verb === 'get' || verb === 'put' || verb === 'post' || verb === 'delete'?
There was a problem hiding this comment.
Also nit: when we enable eslint there's a fair chance this rule makes the cut https://eslint.org/docs/rules/no-use-before-define
which would error here because isHttpVerb is not defined before use on line 33
There was a problem hiding this comment.
In theory, the || chain is O(n) and hasOwnProperty can be O(log n). In practice, it's 4 items so wouldn't be surprised if the || chain is faster.
There was a problem hiding this comment.
function calls, standard library implementations, iterating, garbage collection
A Sufficiently Smart Compiler™ could notice the object literal is static so not allocate it each time and devirtualize the hasOwnProperty call. I have no idea what v8/node actually does under the hood though.
There was a problem hiding this comment.
Oh my, you are quite right!
Now I want to benchmark 😅
Not sure how accurate this is though
There was a problem hiding this comment.
Yeah the main thing I see is that the function param is hardcoded to delete in that benchmark and so it might be optimized in ways that the general version isn't, or maybe not idk.
bitgopatmcl
force-pushed
the
response-in-express-wrapper
branch
2 times, most recently
from
901044d to
31d415d
Compare
| async (req, res) => { | ||
| const maybeRequest = httpRoute.request.decode(req); | ||
| if (maybeRequest._tag === 'Left') { | ||
| console.log('Request failed to decode'); |
There was a problem hiding this comment.
will people be using this function? Cuz this would log in their console right which is something they might not want
There was a problem hiding this comment.
This function is intended to be package-internal. It's exported from this file but not re-exported from index. Does express have some kind of logging system? I don't see one skimming the docs.
There was a problem hiding this comment.
While I ponder this, I will point out the 12-factor way is "log to stdout/stderr"
There was a problem hiding this comment.
I did come across this over the weekend, is it destiny? or inapplicable
https://www.lpalmieri.com/posts/error-handling-rust/
Here we are handling the error on the developer's behalf, and it would be transparent if not for this error. According to this advice, logging here is the proper action
bitgopatmcl
force-pushed
the
response-in-express-wrapper
branch
from
31d415d to
eb5b63e
Compare
| 'hello.world': { | ||
| put: [routeMiddleware, CreateHelloWorld], | ||
| get: [GetHelloWorld], | ||
| put: { middleware: [routeMiddleware], function: CreateHelloWorld }, |
There was a problem hiding this comment.
Seeing it here, is function the clearest and least-ambiguous key? What about handler?
I see the word handler used in this documentation https://expressjs.com/en/guide/using-middleware.html
but I should mention this was a quick search and I'm not sure this is the right terminology.
I feel like we can be more precise in how the function will be used, instead of what it is
There was a problem hiding this comment.
Noticed it is inconsistently named in the codebase too, so this would clean it up.
ericcrosson-bitgo
left a comment
ericcrosson-bitgo
left a comment
There was a problem hiding this comment.
I like this, and I think we should punt the discussion on logging so we can get a clear vision for the entire family of packages to avoid waffling at each log-site.
Only outstanding comment is the one about renaming function -> handler, what do you think there?
bitgopatmcl
dismissed stale reviews from ryan-bitgo and ericcrosson-bitgo
via
5a7dc31
bitgopatmcl
force-pushed
the
response-in-express-wrapper
branch
from
eb5b63e to
5a7dc31
Compare
|
@bitgopatmcl I see a merge conflict |
bitgopatmcl
force-pushed
the
response-in-express-wrapper
branch
from
5a7dc31 to
7dd2fff
Compare
ericcrosson-bitgo
left a comment
ericcrosson-bitgo
left a comment
There was a problem hiding this comment.
Looks good to me 🚢
|
🎉 This PR is included in version 1.0.0-beta.6 🎉 The release is available on npm package (@beta dist-tag) Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 0.2.0-beta.5 🎉 The release is available on npm package (@beta dist-tag) Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 0.2.0-beta.1 🎉 The release is available on npm package (@beta dist-tag) Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 0.2.0-beta.5 🎉 The release is available on npm package (@beta dist-tag) Your semantic-release bot 📦🚀 |
3 participants
This PR does two main things:
responselibrary from several places, cleaning up several parts of the codebaseresponselibrary to still be used for creating http-agnostic service functionsOn the second point, I tried a few approaches before ultimately punting on it and just extending the "decode request and encode response" function in
express-wrapperto handle either raw HTTP status codes orresponseones.