fix: propagate api errors to the terminal correctly.

[threepointone]

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.

Before: (note the generic http error message)

After: (note the cloudflare error code and message)

[changeset-bot]

🦋 Changeset detected

Latest commit: 9dcecb1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
wrangler	Patch

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

[petebacondarwin]

How does this PR achieve, this?

It doesn't throw on non-200 responses

[threepointone]

https://github.com/cloudflare/wrangler2/blob/1e2e27566ebb637b71f17f8a7c73d58455767dae/packages/wrangler/src/cfetch/internal.ts#L34-L41
^ This section would throw when response.ok was false (i.e non 2xx response)

[Electroid]

Look good. Bigger question: should we show a stacktrace for every HTTP error? There are some errors we would expect to be common, like when a script is invalid on upload. How do we differentiate between errors we expect users to run into (which should be nicely formatted), versus an "uncommon" error from the API?

[petebacondarwin]

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.

How about we just change the return type of fetchInternal() from Promise<ResponseType> to Promise<{status:number, statusText?:string, response: ResponseType}>?

Then this information will be available in the fetchResult() (and friends) function, which I think would allow us to propagate that to the upper levels and resolve @Electroid's question?

[threepointone]

This specific error was unexpected, because we should've detected that durable objects weren't configured correctly (which we sort of did, but we proceeded anyway). In general we only log the error .message. And after this PR, we'll have a handle on proper error codes, so we can give much better error messages and next steps for the developer to take.

[threepointone]

I'd be happy to send a PR after landing this one that changes that signature, this PR is simply to do what we already expected the code to do.

[petebacondarwin]

Cool. Happy to approve that this makes the code better.

(We really should have some tests for cfetch stuff, since it is not trivial and is currently mocked out of every test we have.)