-
Notifications
You must be signed in to change notification settings - Fork 590
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
BREAKING(http/server): deprecate serve and serveTls #3381
Conversation
Thanks for the PR. I think we also need to wait for Deploy to support |
Perhaps, |
What version of std should this be removed by? |
I found a few files that use serve or similar and I'm not sure if it makes sense for them to stick around:
Should these be removed? I've updated them to use |
I think so. The points you've raised seem to justify removing them. |
They look ok to remove to me too. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
There are snippets that contain the use of the |
I'm wondering if we should also deprecate |
I think we should. |
I think that's another independent topic. The use case like serving HTTP over unix domain socket listener ( |
Ah, didn't know that. Reverting the deprecations. |
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 |
According the API reference, |
That should be visible with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM again
I'm unsure if we should land this in it's current state with #3381 (comment) still remaining a question. Any thoughts? |
Ah, ok. I missed that.
I think replacing |
Seems reasonable, going to update the PR. |
LGTM now. |
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! |
Ref: https://deno.land/[email protected]/http/server.ts?s=serve `serve()` is deprecated at denoland/std#3381
Closes #3338
Tasks: