qol(routing): warn when api route method doesn't match the casing of an export #9497
Conversation
🦋 Changeset detectedLatest commit: e71cfae The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
pkg: astro
| logger.warn( | ||
| 'router', | ||
| `No API Route handler exists for the method "${method}" for the route ${url.pathname}.\n` + | ||
| `Found handlers: ${Object.keys(mod).map(method => JSON.stringify(method)).join(', ')}\n` | ||
| ); |
lilnasy
force-pushed
the
endpoint-wrong-export-message
branch
from
4e4152d
to
ccd644f
Compare
There was a problem hiding this comment.
Hello! I have never reviewed here, so I am sorry if anything I have said is out of scope.
These changes look very helpful for someone who has just upgraded to v4.0! I especially like the Found handlers: log, which will be great now that there are no deprecation notices on lowercase methods anymore.
I have left a couple of comments on my opinions. I hope they are helpful :)
| '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.
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.
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.
Co-authored-by: Voxel <voxelmc@hotmail.com>
| '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.
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.
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.
There was a problem hiding this comment.
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.
Changes
Testing
Does not affect behavior, just messaging - existing tests should pass.
Docs
Does not affect usage.