Skip to content
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

feat(ext/http): Add addr to HttpServer #23442

Merged
merged 7 commits into from
Apr 20, 2024

Conversation

mmastrac
Copy link
Contributor

@mmastrac mmastrac commented Apr 18, 2024

Adds an addr field to HttpServer to simplify the pattern Deno.serve({ onListen({ port } => listenPort = port }). This becomes: const server = Deno.serve({}); port = server.addr.port.

Changes:

  • Refactors serve overloads to split TLS out (in preparation for landing a place for the TLS SNI information)
  • Adds an addr field to HttpServer that matches the addr field of the corresponding Deno.Listeners.

@mmastrac mmastrac changed the title feat(ext/http): Add addr to HttpServer [WIP] feat(ext/http): Add addr to HttpServer Apr 18, 2024
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question. Can you run deno_std (and maybe fresh) tests against this branch to see if there are any issues with new types?

| ServeTlsOptions
| (ServeTlsOptions & TlsCertifiedKeyOptions),
handler: ServeHandler,
): HttpServer;
handler: ServeTlsHandler,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this specialized interface here? Feels to me like TLS should use ServeHandler

Copy link
Contributor Author

@mmastrac mmastrac Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually will need this eventually as we have to pass the TLS handshake data in. I can push this off for now but we definitely need it (soon).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rolled back the ServeTlsHandler for now, but left ServeTlsInit in-place.

Copy link
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to have! I thought we already had this.

ext/http/00_serve.js Show resolved Hide resolved
@mmastrac
Copy link
Contributor Author

Passed:

  • fresh: deno check:types
  • deno_std deno lint

@mmastrac mmastrac merged commit 365e1f4 into denoland:main Apr 20, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants