-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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(adapter-cloudflare-workers): throws status 404 if KVError instanceof #9509
fix(adapter-cloudflare-workers): throws status 404 if KVError instanceof #9509
Conversation
🦋 Changeset detectedLatest commit: 8907ece The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
0e2868a
to
56ae111
Compare
Thanks for the PR. It's not totally clear to me what effect setting async function get_asset_from_kv(req, env, context, map = mapRequestToAsset) {
try {
return await getAssetFromKV(
{
request: req,
waitUntil(promise) {
return context.waitUntil(promise);
}
},
{
ASSET_NAMESPACE: env.__STATIC_CONTENT,
ASSET_MANIFEST: static_asset_manifest,
mapRequestToAsset: map
}
);
} catch (err) {
- if (err instanceof KVError) err.status = 404;
+ if (e instanceof NotFoundError) {
+ return new Response('Not found', { status: 404 })
+ } else if (e instanceof MethodNotAllowedError) {
+ return new Response('Method not allowed', { status: 405 })
+ } else {
+ return new Response('Internal Error', { status: 500 })
+ }
- throw err;
}
} The code in this PR is somewhat simpler, but is that behaviour documented somewhere? |
3e4787a
to
5191e16
Compare
3da069c
to
141a8c5
Compare
My bad, i was confused. Rich is right. I added the errors that are documented on the documentation of kv-asset-handler. Fixed. |
…ad of 404 for kv assets
141a8c5
to
8907ece
Compare
} else if (e instanceof MethodNotAllowedError) { | ||
return new Response('Method not allowed', { status: 405 }); | ||
} else { | ||
return new Response('Internal Error', { status: 500 }); |
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.
it feels like this might make it hard for people to figure out what the underlying error is
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.
I cant see how to improve this out. Should it throw the error that it catches on the try catch clause🤔 ?
closing this since the issue has been resolved. Completed by #4276 |
Describe the problem
Fixes #9288. The bug occurs when deploying to Cloudflare Workers, where requests for non-existing assets result in a 500 internal error instead of a 404 error.
The Solution
Add a
try-catch
clause to handle the exception if theerror
is an instance of KVError from Cloudflare Workers it will change theerror.status
to404
instead of the default500
as described in the PR.Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.