The error is being generated by OkHTTP, so we should change this in the call site.

I looked into this and OkHttp doesn't make it very easy to fix the caller since the HttpMethods class is internal and that's what they use to determine whether to require a body (or require no body). Changing the caller does look like the right fix though -- three ideas are:

1. Reimplement the logic of HttpMethods
2. Use try-catch in the caller
3. Leave NetworkingModule alone and make Catalyst robust so it can handle the redbox and keep on going. Then adjust the XHR polyfill in JS to pass in a body when we think it's a POST.

Their required RequestBody stuff is such a pain. I had issues with the lack of support to allow DELETE with RequestBody in the past.

Polyfilling the body doesn't sound like a bad idea. Will the error subside if we supply a body argument with an empty string? The issue sounds to primarily exist due to the absence of a request body, versus it being empty, as described in square/okhttp#751. And it doesn't sound like there will be support in the library based on the HTTP spec requiring a request body.

I prefer option 3 in the list above. It's so much better when the native layer is robust and doesn't throw assertions when JS send it unexpected commands, and we also want to make the JS API (XHR polyfill in this case) convenient to use.

Yeah, very valid. The underlying system shouldn't fail for this unexpected input. So it sounds like this should be twofold --

1. Improve Catalyst to not cripple by this unexpected input
2. Add convenience to reduce user error by supplying an empty (rather than undefined) request body.

As a follow-on, it may be good to look at this behavior for other requests that may expect a request body, like PATCH or PUT.

This is the logic that OkHttp uses https://github.com/square/okhttp/blob/master/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpMethod.java#L27

I wish they made this part of their public API so we wouldn't have to replicate it but it's very simple code fortunately.

Yeahhh, i ran into this when debugging the DELETE stuff a while ago. Given that there are these 5 enforcements, I think that when addressing this we may want to cover at least those defined in the HTTP spec, POST, PUT, and PATCH when addressing the polyfill (not just POST alone). Thoughts?

We should handle the other HTTP methods too. I don't feel strongly about the PROPPATCH and REPORT methods but they're simple to support so why not?

On second thought, it would make sense to add polyfill support for those additional 2.
Since RN uses OkHTTP for its networking, and as the networking library requires post bodies for those two off-spec request methods, in the end there is more value in supporting those additional two. Otherwise we're just leaving two edges that are now known to fail based on the requiresRequestBody rule.

+1 on supporting all 5.

Any update here? :) Perhaps @ide

the same error in HTTP PUT request.

Relatedly,

method patch must not have a request body

    if (data == null) {
      if(method.equals("POST")
              || method.equals("PUT")
              || method.equals("PATCH")){
        requestBuilder.method(method, RequestBody.create(null, new byte[0]));
      } else {
        requestBuilder.method(method, null);
      }
    }

@ide this would do?

basically copy-pasted the method 'HttpMethod.requiresRequestBody' from https://github.com/square/okhttp/blob/master/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpMethod.java#L27