Conversation
smorimoto
force-pushed
the
typescript-port
branch
3 times, most recently
from
14adcfe to
2dc8543
Compare
src/chat.ts
Outdated
| // Send the request to the object. The `fetch()` method of a Durable Object stub has the | ||
| // same signature as the global `fetch()` function, but the request is always sent to the | ||
| // object, regardless of the request's URL. | ||
| return roomObject.fetch(newUrl, request); | ||
| return roomObject.fetch(newUrl, { | ||
| method: request.method, | ||
| headers: request.headers, | ||
| }); |
There was a problem hiding this comment.
I'm sceptical that the original code and comments here are correct. (But I often make silly mistakes.)
There was a problem hiding this comment.
The original code was correct. The idea is that we want to send a request that is identical to request except with a modified URL. Since the Request type's properties have the same names as the options object that is passed as the second parameter to fetch(), we can simply pass the request object itself and have it be treated as options.
The new code you've written here only copies over the method and headers from the original request; everything else is lost. In particular, this loses body, as well as some more obscure stuff like referrer.
This is a pretty common pattern in Workers code. Does TypeScript flag this as an error?
There was a problem hiding this comment.
RequestInit allows either IncomingRequestCfProperties or RequestInitCfProperties as the cf field, whereas fetch only allows RequestInitCfProperties. To fix this mismatch, chopping the cf field makes it relatively easy on the user side to let TypeScript take the flag down, but I'm still unsure if this is the right way with Workers.
const { cf, ...requestInit } = request;
return roomObject.fetch(newUrl, requestInit);There was a problem hiding this comment.
Interesting, this seems like a bug in the type definitions. The Request constructor and fetch should have the same argument signature.
There was a problem hiding this comment.
If so, it seems worth raising the issue in the workerd repository and fixing the thing cleanly there.
There was a problem hiding this comment.
@kentonv I think cloudflare/workerd#324 is relevant to this topic.
|
Hi Sora, I appreciate your effort making this example better by porting it to TypeScript! However, this is a very big change, and in order to accept it, we would need to carefully review and test it, which will take some work. I am not sure when I or someone else on the team will be able to get to this. In the meantime though, we can at least leave the PR open so people who want to see a TypeScript version can find it here. |
|
My pleasure. I have a good time playing in the Workers ecosystem. OK, let's keep this open for the time being. |
Signed-off-by: Sora Morimoto <sora@morimoto.io>
Signed-off-by: Sora Morimoto <sora@morimoto.io>
Signed-off-by: Sora Morimoto <sora@morimoto.io>
Signed-off-by: Sora Morimoto <sora@morimoto.io>
smorimoto
force-pushed
the
typescript-port
branch
from
81f8643 to
88d42c8
Compare
Revert changes to checkLimit(). This function was written as intended: the call to `callLimiter()` is intended to run asynchronously, while `checkLimit()` is intended to return synchronously.
|
any update on this ? |
|
Are you still interested in merging this? If so, I can update this to make this mergeable. |
3 participants
This PR ports the main module to TypeScript. There are some mismatches in the original code in fairly strict terms, and I believe TypeScript can make things more explicit.
We use the latest yarn to manage dev dependencies, but I'm open to switching to any package manager.
*Use the "hide whitespace changes" option for easier viewing.