Closed lukeed closed 3 years ago
Ok, so I was having a play in the TS playground a bit, combing some of the ideas in here:
Handler
signature to accept two params: a raw Request
and a Context
objectServerRequest
entirely:
req.params
to context.params
req.search
(and others) to context.url.search
(and others) context.url
is the raw new URL(req.url)
object, so you can reference it wherever/whenever.req.query
-> context.url.searchParams
context.url.query
alias.req.body()
smart helper to a worktop/utils
export.ServerResponse
for you context.res
during the API.prepare
hook.ServerResponse.send
and ServerResponse.end
so that they return Response
directlyNote: You'll have to scroll towards the bottom to see user-facing code!
Closing this as it – well, some form of "it" – has been implemented in #83
This is going to be available as worktop@next
and I will continue to experiment with the new system under these @next
tags. Anyone is welcome to try it out too, of course!
Once I have enough confidence in the new arrangement, this work will be promoted to the main/latest worktop
stable release.
I've been looking at the
ServerRequest
andServerResponse
interfaces a lot lately & wondering if they should even be here 🤔Background
First, an overview of the PROs/CONs of everything, which lays out the reasoning for their existence:
ServerRequest
PROs
ServerRequest<P>
TS interface makes for really nice DX as it can give confidence wrtreq.params
contentsreq.body()
"smart helper" is nice for auto-detecting & auto-parsing the bodyreq.body.json()
(and other) methods, if needednew URL
is done once and its values are stored on therequest
directly, so don't have to repeatCONs
Request
toServerRequest
req.clone
as it no longer existsnew Request(req.url, req)
out of the box (related #52)ws.connect
doesn't have a preferencews.listen
returns a handler than needs to be given aServerRequest
because of thereq.params
usage within itServerRequest
must be created from aFetchEvent
in order forreq.extend
to existFetchEvent
sVisually
Compared to
Request
, these are the property differences:ServerResponse
The main purpose of
ServerResponse
was to surface a Node.js-like API for composing responses.PROs
res.end
,res.setHeader
,res.writeHead
and a bunch of othersnew Response(res.body, res)
forServerResponse
->Response
transformres.end
orres.send
is calledreq.method
for construction – used forHEAD
checksres.send
for automaticContent-*
headers andres.body
serializationCONs
Response
type requirementsServerResponse
to an external library/helper if it wants aResponse
valueServerResponse
is tiny, it's still extra codeHandler
The
Handler
right now is strictly tied to theServerRequest, ServerResponse
pair. It makes sense for this to always have some worktop-specific signature to it, but the question boils down to whether or notServerRequest
andServerResponse
are the right base units.The signature now is this:
...which satisfies all "middleware" and "final/route handler" requirements. Worktop loops through all route handlers until either:
Response
orPromise<Response>
is returned directlyres.send
orres.end
have been called, which marks theServerResponse
as finished, and aResponse
is created from theServerResponse
contentsPROs
next()
CONs
ServerRequest
andServerResponse
to be setup firstWith the background out of the way, I have a few ideas of how these could be simplified and/or reworked to be compatible with libraries outside of worktop.
Request Changes
1. Raw
Request
and add$
object for all customizationsThis drops
ServerRequest
and instead usesRequest
directly, adding a$
property that has an object with all worktop extras. This would be like how Cloudflare adds thecf
key with its own metadata.Breaks:
req.params
->req.$.params
req.path
->req.$.path
req.*
->req.$.*
req.body()
->req.$.body()
req.body.json()
->req.json()
(rely on native)req.body.*()
->req.*()
(rely on native)req.extend()
->req.$.extend()
Additionally, I see two potential issues with this:
Request
interface to a generic, which means code can throw/cause TS errors when used outside of worktop. In other words, if someone tries to doRequest<MyParams>
outside of worktop, then that's gunna throw an error becauseRequest
, natively, is not a generic. Not having the generic means that worktop loses itsparams
insights.req.$
always exists, but if a user writes a middleware/handler to be used outside of worktop, then something likereq.$.path
will throw acannot access "path" of undefined
error2. Raw
Request
and usecontext
object for all customizationsThis is the exact same thing as above, but it uses the
context
key instead of$
for the object.3. Add all customizations directly to a
Request
objectUses the incoming
Request
as is, but adds all the worktop customizations to it directly:Breaks:
req.body()
-> removed – use external utilityreq.body.json()
-> removed – usereq.json()
req.body.*()
-> removed – usereq.*()
Additionally, this shares the same potential gotchas as Options 1 & 2
Request
to be a generic; might cause issues externallyreq.params
object; any user middleware running outside of worktop might includereq.params.foo
andreq.params
is not definedRequest Changes: Poll
Response Changes
There's not much that can really change about
ServerResponse
since it's all custom/worktop's to begin with... Really, there's only been on thing on my mind:Move
res.send
to external utilityIf
res.send
were exported as a top-levelsend
utility (either fromworktop/response
or fromworktop/utils
), then people could use it externally.In order to accomodate this change,
send
would have to return aResponse
directly. Right now it mutates theServerResponse
to signal theworktop.Router
that the Handler loop is complete. All of the serialization &statusCode
checks would happen within thesend
utility itself. However, it would be up to theRouter
to perform theHEAD
check. This would keep its API more or less comparable to@polka/send
.Response Changes: Poll
Handler Changes
As mentioned before, the
Handler
itself depends on decisions made to(Server)Request
and(Server)Response
. So any suggestions here will be made in addition to those, as this section is focusing on theHandler
function signature itself.1. Absolutely no changes
Keep the signature the same and use the exactly same
ServerRequest
andServerResponse
types:2. Keep the signature, but use
Request
typeMaintain the
(req, res)
parameters, but replaceServerRequest
with one of the suggestions above.3. Use
Request+
and aContext
objectChanges the signature so that a
Request
is always used, leaving it up toContext
to store additional/extra information. YourHandlers
will be passing around the same Context object, which allows middleware/handlers (includingRouter.prepare
) to mutate thecontext
as needed.For this Option 3 entry, the
Context
object looks like this:4. Use pure
Request
and aContext
objectSimilar to Option 3, but moves all would-be
ServerRequest
extras into theContext
object. This means that theRequest
is pure and has nothing added onto it (other than Cloudflare'scf
property)5. Only receive a
Context
parameterChange the
Handler
signature so that it's just receiving a single object with everything in it. It may look something like this:Handler Changes: Poll
Now, if you voted for something that involved a
Context
object, should worktop auto-create aServerResponse
for you?All
Context
-based answers assume thatServerRequest
is gone, shifting its properties either to thereq
directly or to theContext
object. In this world, Worktop can continue providing acontext.response
(name TBD) for you, or you can create it yourself as part of yourRouter.prepare
hook.The purists among you may have despised the wannabe-Nodejs all this time, so this would be the opportunity to explicitly opt into
ServerResponse
only when needed ... something like this:Thank you!
I know this a lot (too much) to read and sift through. If you managed to go through it – or even some of it – thank you so much. I really really appreciate the feedback.