BREAKING(http/server): deprecate serve and serveTls

[lino-levan]

Closes #3338

Tasks:

• Deno.serve is stabilized feat: Stabilize Deno.serve() API deno#19141
• Deno.serve is available in Deploy

[kt3k]

Thanks for the PR.

I think we also need to wait for Deploy to support Deno.serve before landing this

[iuioiua]

Perhaps, std/http/file_server.ts should also be adjusted to use Deno.serve() in this PR.

[lino-levan]

What version of std should this be removed by?

[lino-levan]

I found a few files that use serve or similar and I'm not sure if it makes sense for them to stick around:

• http/bench.ts - seems like just a quick file to bench the http server, doesn't really make sense to stick around AFAIK
• http/testdata/simple_https_server.ts - seems to have been used at one point in a test but no longer is referenced anywhere in the codebase. Also doesn't work right now at all since it imports something that doesn't exist.
• http/testdata/simple_server.ts - seems to have been used at one point in a test but is no longer referenced anywhere in the codebase

Should these be removed? I've updated them to use Deno.serve for now.

[iuioiua]

Should these be removed? I've updated them to use Deno.serve for now.

I think so. The points you've raised seem to justify removing them.

[kt3k]

Should these be removed?

They look ok to remove to me too.

[lino-levan]

Deno.serve just landed in deploy. Should now be fine to merge after a review.

[kt3k]

LGTM 👍

[iuioiua]

There are snippets that contain the use of the std/http/server, which I think should be replaced with the use of Deno.serve(). E.g. https://deno.land/[email protected]/http/server_sent_event.ts

[lino-levan]

I'm wondering if we should also deprecate serveListener and the Server class?

[iuioiua]

I think we should.

[kt3k]

I'm wondering if we should also deprecate serveListener and the Server class?

I think that's another independent topic. The use case like serving HTTP over unix domain socket listener (Deno.listen({ path: "/foo/bar.sock", transport: "unix" })) is not covered by Deno.serve.

[lino-levan]

I think that's another independent topic. The use case like serving HTTP over unix domain socket listener (Deno.listen({ path: "/foo/bar.sock", transport: "unix" })) is not covered by Deno.serve.

Ah, didn't know that. Reverting the deprecations.

[lino-levan]

The trouble I'm having is that we should definitely remove some types before 1.0 and replace them with ones that make more sense. For instance

export interface ServeInit extends Partial<Deno.ListenOptions> {
  signal?: AbortSignal;
  onError?: (error: unknown) => Response | Promise<Response>;
  onListen?: (params: { hostname: string; port: number }) => void;
}

is used for serve and serveListener. With serve being dropped, this type will only matter for serveListener. serveListener uses the type as Omit<ServeInit, "port" | "hostname">. It doesn't make sense for this type to extend Deno.ListenOptions if the only reference to it in the future will specifically omit all of the types that Deno.ListenOptions provides.

[iuioiua]

I think that's another independent topic. The use case like serving HTTP over unix domain socket listener (Deno.listen({ path: "/foo/bar.sock", transport: "unix" })) is not covered by Deno.serve.

According the API reference, Deno.listen() doesn't support UNIX sockets. Neither do serveListener() and Server. Is this right?

[kt3k]

@iuioiua

According the API reference, Deno.listen() doesn't support UNIX sockets. Neither do serveListener() and Server. Is this right?

That should be visible with Show Unstable API checked

[kt3k]

LGTM again

[lino-levan]

I'm unsure if we should land this in it's current state with #3381 (comment) still remaining a question. Any thoughts?

[kt3k]

Ah, ok. I missed that.

With serve being dropped, this type will only matter for serveListener. serveListener uses the type as Omit<ServeInit, "port" | "hostname">. It doesn't make sense for this type to extend Deno.ListenOptions if the only reference to it in the future will specifically omit all of the types that Deno.ListenOptions provides.

I think replacing Omit<ServeInit, "port" | "hostname"> with its own interface (such as ServeListenerOptions) with only necessary props copied from ServeInit makes more sense.

[lino-levan]

I think replacing Omit<ServeInit, "port" | "hostname"> with its own interface (such as ServeListenerOptions) with only necessary props copied from ServeInit makes more sense.

Seems reasonable, going to update the PR.

[lino-levan]

LGTM now.

[kt3k]

Tested this branch's version of file_server https://raw.githubusercontent.com/denoland/deno_std/b2a0642521f5323a9bd9e772fafc3f006ac7e936/http/file_server.ts on Deploy, and it seemed working https://small-hawk-66-j5jx0nq05yh0.deno.dev/

I think this is ready to land.

Thank you for your contribution!