feat: Stabilize Deno.serve() API

[bartlomieju]

No description provided.

[lino-levan]

The actual PR LGTM.

One thing I am concerned about is the type signature of the method. I don't recall another API in which the options argument is first. iirc there was a PR to remove this option, but perhaps serve(handler, options) would make more sense? Just trying to get some clarity on this decision, don't really have a strong opinion either way.

[bartlomieju]

The actual PR LGTM.

One thing I am concerned about is the type signature of the method. I don't recall another API in which the options argument is first. iirc there was a PR to remove this option, but perhaps serve(handler, options) would make more sense? Just trying to get some clarity on this decision, don't really have a strong opinion either way.

IIRC no other Deno API accepts a callback, in case of Deno.serve() lots of people define handler inline, which often spans multiple lines. In such case it's very easy to miss that after the handler there's also an options bag.

[lino-levan]

In such case it's very easy to miss that after the handler there's also an options bag.

That reasoning makes sense to me, thanks for clarifying!

[lucacasonato]

Also, there is precedent for options bag first: Deno.test.

[bartlomieju]

Also, there is precedent for options bag first: Deno.test.

Doh, of course Deno.test and Deno.bench take callbacks 😅

[bartlomieju]

I have to postpone this PR due to #19189.

[mmastrac]

@lucacasonato pointed out some documentation issues -- I compared the current version of the HTTP server docs with what will be required. For the most part we should be able to swap out serveHttp for serve, but there are some additional documentation items that will have to change

#19306

[kt3k]

A few questions:

• Is port 9000 sensible default? (Currently std's serve defaults to 8000. If Deno.serve is the successor of it, maybe we should keep 8000 as default?)
• Is the 3rd overload really necessary? (The difference of the 2nd and the 3rd looks marginal)

[bartlomieju]

A few questions:

• Is port 9000 sensible default? (Currently std's serve defaults to 8000. If Deno.serve is the successor of it, maybe we should keep 8000 as default?)

Sure, we can change it to be 8000.

• Is the 3rd overload really necessary? (The difference of the 2nd and the 3rd looks marginal)

We discussed overloads two months ago and already removed some of them. @lucacasonato was rather adamant on keeping these 3.

[ry]

these docs are repeated for each overload?

[bartlomieju]

I think that's a requirement for deno_doc

[ry]

Maybe each doc should be specific to the overload? Or maybe we should just consider that a bug in deno_doc and do what makes sense in the d.ts file. It does not make sense to duplicate documentation three times - so there's definitely a bug here (either in deno_doc or jsdoc spec)

[bartlomieju]

CC @dsherret @crowlKats do you remember the reason why this was needed? Can we somehow flatten all three overloads to be next to each other with only a single doc?

[crowlKats]

a doc block is specific to its overload; there is no way to specify a single doc for all overloads. unsure how we could change this, as it is favourable to have the capability to document overloads differently from each other.

[bartlomieju]

I've updated the docs so that each overload has a different doc string.

[lucacasonato]

LGTM. Great work to everyone involved!

Let's make sure to update the manual after this lands.

[0f-0b]

serve is still listed in denoNsUnstable and is not present at runtime without --unstable.

deno/runtime/js/90_deno_ns.js

Line 174 in 686ec85

serve: http.serve,

$ deno --version
deno 1.34.3+686ec85 (canary, x86_64-apple-darwin)
v8 11.6.189.7
typescript 5.1.6
$ deno eval 'console.log(Deno.serve)'
undefined
$ deno eval --unstable 'console.log(Deno.serve)'
[Function: serve]

[bartlomieju]

Wow, thanks for pointing this out @0f-0b, fix incoming.