-
Notifications
You must be signed in to change notification settings - Fork 76
fix: Refactor promise check for response handling #277
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
I got an error because `instanceof` wouldn't recognize `res` as a `Promise`. Might be related to my usage of `tsx`, idk. Anyway, if I change to check for a `then` method, everything works again.
|
Oh, sorry, I accidentally approved it by mistake. |
|
Hi @kay-is, thanks for creating the pull request. I agree with the change to treat Thenable as a Promise. I think the type error issue can be resolved like this. diff --git i/src/listener.ts w/src/listener.ts
index 2724ffd..a9ea3dd 100644
--- i/src/listener.ts
+++ w/src/listener.ts
@@ -98,12 +98,15 @@ const responseViaCache = async (
;(outgoing as OutgoingHasOutgoingEnded)[outgoingEnded]?.()
}
+const isPromise = (res: Response | Promise<Response>): res is Promise<Response> =>
+ typeof (res as Promise<Response>).then === 'function'
+
const responseViaResponseObject = async (
res: Response | Promise<Response>,
outgoing: ServerResponse | Http2ServerResponse,
options: { errorHandler?: CustomErrorHandler } = {}
) => {
- if (res instanceof Promise) {
+ if (isPromise(res)) {
if (options.errorHandler) {
try {
res = await resCould you please add the tests? Then I think we can merge it. |
|
@usualoma sure thing! |
|
Hi @kay-is, thank you for the PR.
Can you reproduce it? If so, it's better to add a test to confirm that the issue will be resolved with this fix. If it's difficult, we can treat this change as a |
|
@yusukebe sorry, no idea exactly how that error happened or how to reproduce it. I just had a codebase where I assume it has something to do with the |
yusukebe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
No problem! If so, let's go without tests. This is like a refactoring but resolving your issue. So, I'll release a new patch version after merging. Thank you for your contribution! |
I got an error because
instanceofwouldn't recognizeresas aPromise.Might be related to my usage of
tsx, idk if it breaks the prototype chain or something.Anyway, if I change to check for a
thenmethod, everything works again.