-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
fix(ext/http): registerDeclarativeServer #26606
base: main
Are you sure you want to change the base?
Conversation
Added test case for registerDeclarativeServer like we discussed over email
test of registerdeclarativeserver changes
@@ -882,7 +882,7 @@ function registerDeclarativeServer(exports) { | |||
: ""; | |||
const host = formatHostName(hostname); | |||
|
|||
// deno-lint-ignore no-console | |||
// Log server information |
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.
You will need to keep the previous lint supression here
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.
This comment is still relevant and the CI won't pass unless it's addressed
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.
Looks good! You'll also need to update cli/tsc/dts/lib.deno.ns.d.ts
and the ServeDefaultExport
to include new onListen
property that has the same signature as you can find in ServeOptions
interface
export const fetch = (req: Request) => { | ||
return new Response("Hello from declarative server!"); | ||
}; |
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.
We don't support this syntax (yet!), so you'll need something like this:
export default {
fetch(req: Request) {
return new Response("Hello from declarative server");
},
onListen(info) {
console.log(info);
}
} satisfies Deno.ServeDefaultExport;
Co-authored-by: Bartek Iwańczuk <[email protected]> Signed-off-by: ricfrst <[email protected]>
Signed-off-by: ricfrst <[email protected]>
Signed-off-by: ricfrst <[email protected]>
cli/tsc/dts/lib.deno.ns.d.ts
Outdated
/** Interface for serving HTTP requests. */ | ||
interface ServeDefaultExport { |
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.
Looks like we got two ServeDefaultExport
now :D can you please move onListen
declaration to the existing interface on line 5068?
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.
Will do, working on it ASAP
Signed-off-by: ricfrst <[email protected]>
Signed-off-by: ricfrst <[email protected]>
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.
This looks good 👍 just make sure to run tools/format.js
to make CI pass.
Changes to registerDeclarativeServer as discussed over our call