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.

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

Merged
merged 5 commits into from
Dec 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/tidy-dogs-remain.md
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.
2 changes: 1 addition & 1 deletion packages/astro/src/@types/astro.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2235,7 +2235,7 @@ export type APIRoute<Props extends Record<string, any> = Record<string, any>> =
) => Response | Promise<Response>;

export interface EndpointHandler {
[method: string]: APIRoute | ((params: Params, request: Request) => Response);
[method: string]: APIRoute;
}

export type Props = Record<string, unknown>;
Expand Down
46 changes: 15 additions & 31 deletions packages/astro/src/runtime/server/endpoint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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` : '')
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Member

@ematipico ematipico Dec 22, 2023

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message would only be shown when pages/x.ts existed but /x failed to match a handler. I think at that point, it's safe to say the user could use a helpful nudge.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be more precise, I talk about the all check. We don't support that anymore, so we shouldn't write new code around that. New developers would see this line and won't understand why it is there.

);
// 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);
}
Loading