Skip to content

Commit

Permalink
fix: propagate api errors to the terminal correctly. (#287)
Browse files Browse the repository at this point in the history
* fix: propagate api errors to the terminal correctly.

Any errors embedded in the response from the Cloudflare API were being lost, because `fetchInternal()` would throw on a non 200 response. This PR fixes that behaviour:
- It doesn't throw on non-200 responses
- It first gets the response text with `.text()` and converts it to an object with `JSON.parse`, so in case the api returns a non json response, we don't lose response we were sent.

Unfortunately, because of the nature of this abstraction, we do lose the response `status` code and `statusText`, but maybe that's acceptable since we have richer error information in the payload. I considered logging the code and text to the terminal, but that may make it noisy.

* Add a changeset
  • Loading branch information
threepointone authored Jan 24, 2022
1 parent 1e2e275 commit b63efe6
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 6 deletions.
12 changes: 12 additions & 0 deletions .changeset/tall-cheetahs-juggle.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
"wrangler": patch
---

fix: propagate api errors to the terminal correctly

Any errors embedded in the response from the Cloudflare API were being lost, because `fetchInternal()` would throw on a non-200 response. This PR fixes that behaviour:

- It doesn't throw on non-200 responses
- It first gets the response text with `.text()` and converts it to an object with `JSON.parse`, so in case the api returns a non json response, we don't lose response we were sent.

Unfortunately, because of the nature of this abstraction, we do lose the response `status` code and `statusText`, but maybe that's acceptable since we have richer error information in the payload. I considered logging the code and text to the terminal, but that may make it noisy.
12 changes: 10 additions & 2 deletions packages/wrangler/src/cfetch/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,9 +94,17 @@ function throwFetchError(
const errors = response.errors
.map((error) => `${error.code}: ${error.message}`)
.join("\n");
throw new Error(`Failed to fetch ${resource} - ${errors}`);
const error = new Error(`Failed to fetch ${resource} - \n${errors}`);
// add the first error code directly to this error
// so consumers can use it for specific behaviour
const code = response.errors[0]?.code;
if (code) {
//@ts-expect-error non-standard property on Error
error.code = code;
}
throw error;
}

function hasCursor(result_info: unknown): result_info is { cursor: string } {
return (result_info as { cursor } | undefined)?.cursor !== undefined;
return (result_info as { cursor: string } | undefined)?.cursor !== undefined;
}
10 changes: 6 additions & 4 deletions packages/wrangler/src/cfetch/internal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,13 @@ export async function fetchInternal<ResponseType>(
headers,
});

if (response.ok) {
return (await response.json()) as ResponseType;
} else {
const jsonText = await response.text();
try {
const json = JSON.parse(jsonText);
return json as ResponseType;
} catch (e) {
throw new Error(
`Failed to fetch ${resource} - ${response.status}: ${response.statusText}`
`Failed to fetch ${resource} - ${response.status}: ${response.statusText}\nInvalid JSON response:\n${jsonText}`
);
}
}
Expand Down

0 comments on commit b63efe6

Please sign in to comment.