-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
qol(routing): warn when api route method doesn't match the casing of an export #9497
Changes from all commits
658cab5
ef7a412
ccd644f
17833f4
e71cfae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'astro': patch | ||
--- | ||
|
||
Adds a helpful warning message for when an exported API Route is not uppercase. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,18 +2,6 @@ import { bold } from 'kleur/colors'; | |
import type { APIContext, EndpointHandler, Params } from '../../@types/astro.js'; | ||
import type { Logger } from '../../core/logger/core.js'; | ||
|
||
function getHandlerFromModule(mod: EndpointHandler, method: string) { | ||
// If there was an exact match on `method`, return that function. | ||
if (mod[method]) { | ||
return mod[method]; | ||
} | ||
if (mod['ALL']) { | ||
return mod['ALL']; | ||
} | ||
// Otherwise, no handler found. | ||
return undefined; | ||
} | ||
|
||
/** Renders an endpoint request to completion, returning the body. */ | ||
export async function renderEndpoint( | ||
mod: EndpointHandler, | ||
|
@@ -23,38 +11,34 @@ export async function renderEndpoint( | |
) { | ||
const { request, url } = context; | ||
|
||
const chosenMethod = request.method?.toUpperCase(); | ||
const handler = getHandlerFromModule(mod, chosenMethod); | ||
if (!ssr && ssr === false && chosenMethod && chosenMethod !== 'GET') { | ||
const method = request.method.toUpperCase(); | ||
// use the exact match on `method`, fallback to ALL | ||
const handler = mod[method] ?? mod['ALL']; | ||
if (!ssr && ssr === false && method !== 'GET') { | ||
logger.warn( | ||
null, | ||
"router", | ||
`${url.pathname} ${bold( | ||
chosenMethod | ||
method | ||
)} requests are not available for a static site. Update your config to \`output: 'server'\` or \`output: 'hybrid'\` to enable.` | ||
); | ||
} | ||
if (!handler || typeof handler !== 'function') { | ||
if (typeof handler !== 'function') { | ||
logger.warn( | ||
'router', | ||
`No API Route handler exists for the method "${method}" for the route ${url.pathname}.\n` + | ||
`Found handlers: ${Object.keys(mod).map(exp => JSON.stringify(exp)).join(', ')}\n` + | ||
('all' in mod ? `One of the exported handlers is "all" (lowercase), did you mean to export 'ALL'?\n` : '') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We removed the support lower-case functions in v4 (they were deprecated in v3), so we shouldn't show this particular line, otherwise it sounds like we still support it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This message would only be shown when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be more precise, I talk about the |
||
); | ||
// No handler found, so this should be a 404. Using a custom header | ||
// to signal to the renderer that this is an internal 404 that should | ||
// be handled by a custom 404 route if possible. | ||
let response = new Response(null, { | ||
return new Response(null, { | ||
status: 404, | ||
headers: { | ||
'X-Astro-Response': 'Not-Found', | ||
}, | ||
}); | ||
return response; | ||
} | ||
|
||
const proxy = new Proxy(context, { | ||
get(target, prop) { | ||
if (prop in target) { | ||
return Reflect.get(target, prop); | ||
} else { | ||
return undefined; | ||
} | ||
}, | ||
}) as APIContext & Params; | ||
|
||
return handler.call(mod, proxy, request); | ||
return handler.call(mod, context); | ||
} |
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.
Just wondering why
all
is the only case to handle, and not any of the other verbs?I would assume that, if the user reads "get" in the
Found handlers:
log, it should be enough.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.
The most recent user to run into this was exporting "all". I agree this message should expand to "get", "post" depending what the current request's method is.