-
Notifications
You must be signed in to change notification settings - Fork 3
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
Initial Implementation #1
Conversation
src/DenoHTTPWorker.ts
Outdated
}; | ||
|
||
class DenoHTTPWorker { | ||
private _httpSession: http2.ClientHttp2Session; |
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.
There are both TypeScript and JavaScript versions of private properties - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Private_properties - TypeScript's are just forbidding your own code from accessing the "private" variable whereas JavaScript's make it really private - probably we should prefer JavaScript private.
453cce8
to
95f52c2
Compare
@tmcw requesting re-review on this one. A note on a change since your last review: Deno actually makes this pretty easy. You can do the following and only accept a single connection: const server = Deno.listen({
hostname: "0.0.0.0",
port: 0,
});
// Accept the first connection and serve HTTP.
const conn = await server.accept();
const httpConn = Deno.serveHttp(conn);
for await (const requestEvent of httpConn) {
// Handle the HTTP request event.
}
// Close the server to reject all future connections.
server.close(); The issue here is that using the This would not be a big deal, but Deno prints a warning in the logs:
This can be disabled with I guess we could clean it from the logs ourself. I'm going to explore using unix sockets as an alternative because it might skip this issue and provide additional security, but fyi this is the state of the lib for now. |
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.
I will read the rest of this in a bit, but pushing comments now just to keep the ball rolling
const handler = await import(importURL); | ||
if (!handler.default) { | ||
throw new Error("No default export found in script."); | ||
} | ||
if (typeof handler.default !== "function") { | ||
throw new Error("Default export is not a function."); | ||
} |
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.
We'd still have a guest.ts
or similar, right, because we have different rules for exports there - the first import is taken and it doesn't have to be default.
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.
Ok yes, I was going to ask you about this. Correct we'd still have guest.ts. I just wrote up something that worked, but wasn't sure what might be best here. I'll extend it to support the first export as well? Or just leave it as-is because at this layer it's just whatever works for us?
No description provided.