fix: umi.downloader.downloadJson can return a string if content-type is not set on response

[KartikSoneji]

The downloadJson() function should try to parse the response as json even if the content-type header is missing or not application/json.
Some providers like nftstorage.link do not set the content-type header for some reason, resulting in the response being a string.

This PR makes downloadJson() try to parse the response as a json instead of silently returning a string.

[changeset-bot]

⚠️ No Changeset found

Latest commit: c334743

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[lorisleiva]

Hmm what if the response is an actual JSON string though? Like the original body was "[1,2,3]" which should resolve to a string but with this change ends up being an array.

We initially requested an application/json content-type on the request but unfortunately this resulted in lots of CORS issues as any JSON request will trigger a CORS preflight even for GET requests.

I don't think there's much we can do about this tbh if we want to maintain the integrity of valid JSON responses.

[KartikSoneji]

Hmm what if the response is an actual JSON string though? Like the original body was "[1,2,3]" which should resolve to a string but with this change ends up being an array.

That's a fair point.
I can duplicate the check in http.send().

I don't think there's much we can do about this tbh if we want to maintain the integrity of valid JSON responses.

If the user explicitly requests fetching the data as a json I think we should try to parse it as such, if that's not possible at least print a warning and ideally throw an error, but for backward compatibility we can return a string.

[lorisleiva]

I can duplicate the check in http.send().

Isn't that going to be the same issue just somewhere else? Unless I'm misunderstanding what you're suggesting. 😄

[KartikSoneji]

Ideally we would modify HttpResponse to include a json field, but I understand that would break compatibility:

diff --git a/packages/umi-downloader-http/src/createHttpDownloader.ts b/packages/umi-downloader-http/src/createHttpDownloader.ts
index d007c71..5e88bcf 100644
--- a/packages/umi-downloader-http/src/createHttpDownloader.ts
+++ b/packages/umi-downloader-http/src/createHttpDownloader.ts
@@ -34,8 +34,8 @@ export function createHttpDownloader(
       request().get(uri).withAbortSignal(options.signal)
     );
 
-    let json = response.data;
-    if (typeof json === 'string')
+    let json = response.json;
+    if (!response.json && response.body)
       try {
         json = JSON.parse(response.body);
       } catch {
diff --git a/packages/umi-http-fetch/src/createFetchHttp.ts b/packages/umi-http-fetch/src/createFetchHttp.ts
index cb5ecb5..26b7a98 100644
--- a/packages/umi-http-fetch/src/createFetchHttp.ts
+++ b/packages/umi-http-fetch/src/createFetchHttp.ts
@@ -53,6 +53,7 @@ export function createFetchHttp(): HttpInterface {
       return {
         data: bodyAsJson ?? bodyAsText,
         body: bodyAsText,
+        json: bodyAsJson,
         ok: response.ok,
         status: response.status,
         statusText: response.statusText,

Or, we can do the same check in downloadJson().
The downside is both implementations will need to be kept in sync:

diff --git a/packages/umi-downloader-http/src/createHttpDownloader.ts b/packages/umi-downloader-http/src/createHttpDownloader.ts
index d007c71..3105074 100644
--- a/packages/umi-downloader-http/src/createHttpDownloader.ts
+++ b/packages/umi-downloader-http/src/createHttpDownloader.ts
@@ -35,7 +35,7 @@ export function createHttpDownloader(
     );
 
     let json = response.data;
-    if (typeof json === 'string')
+    if (!response.headers.get('content-type')?.includes('application/json'))
       try {
         json = JSON.parse(response.body);
       } catch {

[lorisleiva]

The first solution is not ideal because it will try to unnecessarily parse the response as JSON on every request.

The second solution is interesting though. Because we know we are requesting JSON, if the response was not flagged as a JSON response, we know it wasn't parsed and we're going around the double parsing issue. I'd be happy to merge a PR for this second solution.

[KartikSoneji]

Hmm both should be identical I think?
The diff is a bit difficult to follow, can you please apply it with git apply and check the results?

[KartikSoneji]

Hi @lorisleiva , just a gentle reminder to look into this.

[lorisleiva]

Hey, sorry your previous comment confused me. IMO, the second solution should work and the conditions should not be identical but opposite. I've applied the patches accordingly. Let me know what you think and maybe create a new PR with the final solution?

[KartikSoneji]

Sorry what I meant is both sets of changes should function identically in respect to json parsing.

Ideally, we would return a separate json field that would only be populated if the response had a content-type: json header,
but that would mean modifying the existing interface.

diff --git a/packages/umi/src/HttpResponse.ts b/packages/umi/src/HttpResponse.ts
index b5ae1ff..073e9b3 100644
--- a/packages/umi/src/HttpResponse.ts
+++ b/packages/umi/src/HttpResponse.ts
@@ -7,6 +7,7 @@ import type { HttpResponseHeaders } from './HttpHeaders';
 export type HttpResponse<D = any> = {
   data: D;
   body: string;
+  json?: D;
   ok: boolean;
   status: number;
   statusText: string;

If that is acceptable, then it avoids duplicating the header check in the downloadJson() function.
It also makes downloadJson() implementation agnostic since another fetch provider might use a different criteria for parsing json.

---

If we cannot modify the HttpResponse interface then we can perform the same check as the default fetch provider, and only force json parsing if the send() method didn't already do that.

---

Either way, both solutions should have no extra overhead with respect to json parsing.
The only difference is how the check is performed.

I hope that makes sense, please let me know if you have any questions.