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.

Sign up for GitHub

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

Jump to bottom

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

Open
lowlighter opened this issue Aug 31, 2024 · 4 comments · May be fixed by #6094
Open

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

lowlighter opened this issue Aug 31, 2024 · 4 comments · May be fixed by #6094
Labels
enhancement New feature or request feedback welcome We want community's feedback on this issue or PR good first issue Good for newcomers http PR welcome A pull request for this issue would be welcome

Comments

@lowlighter
Copy link
Contributor

The Handler interface is currently defined as:

std/http/route.ts

Lines 14 to 18 in 94a7e1b

export type Handler = (
request: Request,
info?: Deno.ServeHandlerInfo,
params?: URLPatternResult | null,
) => Response | Promise<Response>;

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:

std/http/route.ts

Lines 93 to 96 in 94a7e1b

const match = route.pattern.exec(request.url);
if (match && request.method === (route.method ?? "GET")) {
return route.handler(request, info, match);
}

So users need to add an uneeded !. since to avoid typing issues
@iuioiua
Copy link
Contributor

iuioiua commented Sep 2, 2024

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?

@iuioiua iuioiua added enhancement New feature or request good first issue Good for newcomers http PR welcome A pull request for this issue would be welcome feedback welcome We want community's feedback on this issue or PR labels Sep 2, 2024
@kt3k
Copy link
Member

kt3k commented Sep 3, 2024

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
Copy link

Leokuma commented Sep 4, 2024

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
Copy link
Contributor Author

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

@lowlighter lowlighter linked a pull request Oct 5, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request feedback welcome We want community's feedback on this issue or PR good first issue Good for newcomers http PR welcome A pull request for this issue would be welcome
Projects
None yet
Milestone
No milestone
4 participants
@kt3k @Leokuma @lowlighter @iuioiua