fix: add "request" to response events #1993
Conversation
| @@ -30,6 +30,8 @@ export const createRequestListener = ( | |||
| const request = parseWorkerRequest(message.payload) | |||
| const requestCloneForLogs = request.clone() | |||
|
|
|||
| context.requests.set(requestId, request.clone()) | |||
There was a problem hiding this comment.
I'm not sure what I feel in particular about this approach but I've been meaning to provide in-memory requestId/request storage for some time now. This will simplify the usage of life-cycle events.
| @@ -103,6 +103,7 @@ export interface SetupWorkerInternalContext { | |||
| worker: ServiceWorker | null | |||
| registration: ServiceWorkerRegistration | null | |||
| requestHandlers: Array<RequestHandler> | |||
| requests: Map<string, Request> | |||
There was a problem hiding this comment.
I would very much like this to be a WeakMap so we wouldn't have to clean it up but then to access the request instance it'd have to be a WeakMap<Request, string>, iterating over which would be rather inefficient.
@mattcosta7, what do you think about this? Is it worth rewriting this as WeakMap?
There was a problem hiding this comment.
Hmm
I think we could, but it might be a bit complicated, and not worth the effort?
I think we would need to WeakMap<Request, {requestId, RequestClone> to really make that change, which might be ok, but it's pretty straightforward to cleanup, and should generally be small enough at any point in time to not be an issue?
| @@ -30,6 +30,8 @@ export const createRequestListener = ( | |||
| const request = parseWorkerRequest(message.payload) | |||
| const requestCloneForLogs = request.clone() | |||
|
|
|||
| context.requests.set(requestId, request.clone()) | |||
There was a problem hiding this comment.
This also means +1 request clone per request.
There was a problem hiding this comment.
A few important notes:
- This is always called once per 1 request.
- Request resolution hasn't started yet so there's no cache.
We can technically make this the earliest clone to prevent the first request handler from cloning this request again. I think this makes sense. Wdyt, @mattcosta7?
There was a problem hiding this comment.
I've added this first request clone to RequestHandler.cache so no request handlers would have to clone the request.
It's still a bit of an overhead since now we are always making 1 clone for every outgoing request (in the browser). Previously, only the first request handler would make the clone. I'd say this is not a concern since request cloning was done in the parsing phase, and the only scenario when this actually changes anything is if you have 0 request handlers (than, indeed, no cloning is needed).
But we still have to clone the request even in that scenario so you can access it in the response:* life-cycle events.
There was a problem hiding this comment.
Yes, I think this is definitely worth it, and the cost seems low enough that it shouldn't be a concern
There was a problem hiding this comment.
Now request handlers won't clone requests at all, I think. Any outgoing request will be cloned here, and then request handlers will reuse the clone from cache.
kettanaito
force-pushed
the
fix/request-argument
branch
from
f187aba
to
30dcf0c
Compare
Released: v2.1.5 🎉This has been released in v2.1.5! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |
This is actually a feature. I've added the
requestargument to the response events by mistake, trying to keep things consistent. But I've never implemented the thing.