denoland / std

The Deno Standard Library
https://jsr.io/@std
MIT License
3.22k stars 622 forks source link

qol(http/route): typings and handler signature #5884

Open lowlighter opened 2 months ago

lowlighter commented 2 months ago

The Handler interface is currently defined as: https://github.com/denoland/std/blob/94a7e1b34be4b2f18af1238d7b62c1a0cc700f41/http/route.ts#L14-L18

But I feel like people users using this API would most likely prefer to have URLPatternResult as second argument and Deno.ServeHandlerInfo as third one.

Reasoning is that people using this API will almost always want to retrieve their route params, but the usage of Deno.ServeHandlerInfo is less likely. Also since the url/method is already checked by the route() function, it is also common not have have use for Request, so you may end up with:

handler: (_, __, result) => return new Response(result.pathname.groups.name)

Which is less elegant than:

handler: (_, result) => return new Response(result.pathname.groups.name)

I understand it was done to be compatible with regular handlers, but maybe route() could support an union of both.


Also the typing params?: URLPatternResult | null, seems incorrect (at least for non-default handler), because the result is checked before being passed down to handler: https://github.com/denoland/std/blob/94a7e1b34be4b2f18af1238d7b62c1a0cc700f41/http/route.ts#L93-L96

So users need to add an uneeded !. since to avoid typing issues

iuioiua commented 2 months ago

These are good points. Yeah, maybe route() should support an added, more elegant, overload with params before info. The typing of params could be better too. I'd happily take a look at PRs that implemented these changes. Opinions, @kt3k and @marvinhagemeister?

kt3k commented 2 months ago

Generally sounds good to me.

But I'm a bit concerned about that deno init --serve output will be broken for exsting Deno versions.

@lowlighter

maybe route() could support an union of both.

What is the union of both?

Leokuma commented 2 months ago

But I'm a bit concerned about that deno init --serve output will be broken for exsting Deno versions.

Sounds concerning not only for now but also for future maintenance of deno init in general.

Maybe deno init shouldn't even exist. We could have a "deno-templates" or "project-starter" package in Std that would have a collection of init scripts to achieve the same thing that deno init does, with the benefit that it's more flexible, can take the Deno version into account, doesn't bloat the runtime and keeps Deno's codebase and CLI minimal.

lowlighter commented 2 months ago

I agree with @Leokuma, maybe the deno init should just be an alias that list available examples from @std/examples or docs.deno.com/examples

Anyways changing the signature will indeed break the deno init example. However, as the route() is still marked "unstable", I assumed it was for end-users to play with it and gather feedback and perform breaking changes before a possible stabilization. I feel like deno init shouldn't have used an "unstable" feature in the first place.

Or else it could be a new major (the deno init example is tagged with @1) to avoid breaking it, but feels weird to do it for something unstable


@kt3k

What is the union of both?

I forgot it was dealing with functions so I yeah I assume it's not possible to achieve since there's no way to discriminate them at runtime