Improve fetchBaseQuery content json verification

[danibonilha]

Currently in the docs regarding the body it says:
// fetchBaseQuery automatically adds 'content-type: application/json' to the Headers and calls JSON.stringify(patch)
However it doesn't state that it only "stringifies" if the content-type is strictly application/json, I use application/vnd.api+json for example and it was not very easy to figure what was wrong.

My proposal is to check if the content-type contains the json extension, so it will cover all json cases, if no one sees any issues with this proposal I'd be glad to push a PR for it, otherwise we could at least update the docs to avoid any confusion.

[danibonilha]

@phryneas any thoughts ?

[phryneas]

I honestly have no idea, all those additional content-types are pretty much out of my knowledge domain since I never had to touch any of them.

If it is a valid thing: sure, why not? But are we 100% sure that it is valid for every last one?

[ShaunDychko]

I ran into this same issue also because of using the application/vnd.api+json media type. When the client communicates with some endpoints expecting Content-Type: application/vdn.api+json and other endpoints expecting Content-Type: application/json, this issue creates the need to apply JSON.stringify() to the body manually in the endpoint definition when communicating with JsonApi, but then to not apply JSON.stringify() when communicating with a regular JSON endpoint.

The iana list of media type shows about 136 media types containing json, and all but one start with application/ and end with json, so the pull request detects the content-type accordingly.

[phryneas]

My problem is that already the first one on that list (application/3gppHal+json) says Encoding considerations: binary in the spec. I'm not sure that all of them actually use text-based json.

[ShaunDychko]

How about matching either application/json or application/vnd.api+json? There's probably reluctance to maintain a list of text encoded json media types, but perhaps JSON API is popular enough, based on this list of implementations to warrant inclusion? For what it's worth application/json also has Encoding considerations: binary.

The goal with this issue is to try and prevent other developers from encountering an issue where sometimes the body in their endpoint definition has JSON.stringified() applied automatically (where Content-Type: application/json), but then is not applied even though they might expect it when Content-Type: application/vnd.api+json. Using Drupal as a backend with JSON API it was necessary to figure out that requests to JSON API needed JSON.stringify() in the endpoint definition, whereas if JSON.stringify() was applied in the endpoint definition to a regular JSON endpoint, it would end up being stringified twice and could not be deserialized by the endpoint (Drupal).

Another approach, if the goal is to prevent applying JSON.stringify() twice, could be to see if the body isJsonifiable, so I've included a different PR with that approach.

[msutkowski]

@ShaunDychko @phryneas I ran into this recently with a custom fetch wrapper based around fetchBaseQuery, and added the solution as proposed here: #2331. I'd recommend we go with this approach as it's a simple opt-in mechanism for JSON API users (like me, sometimes! 😂)

[ShaunDychko]

#2331 is definitely better with the added flexibility. Since we're here on account of application/vnd.api+json, and that media type will very likely need to have the body stringified, I made one last plug to include application/vnd.api+json by default in a PR review.

[ShaunDychko]

Oh, on the question of including application/vnd.api+json by default there's some juicy irony: it's an anti-bikeshedding specification! 🤣

[phryneas]

This is handled in #2331 which I just merged into the 1.9 branch.