Do not serialize body values that aren't objects (like FormData)

[heralden]

Would love some feedback on this. Not sure if my solution is optimal as I'm not that familiar with this.

When using RESTDataSource while setting up apollo-server and learning GraphQL, I had trouble getting my POST requests to work. Looking through the sources, I saw that the fetch method in RESTDataSource was running JSON.stringify on my FormData, causing our backend server to reject them.

I don't think this should be by design? I saw that there was coded in an exception for ArrayBuffer, but to add FormData would perhaps require pulling in the form-data dependency, which isn't very practical. So I thought we might as well only serialize body values that are javascript objects. (constructor === Object) Does this make sense?

[apollo-cla]

@uosl: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

[martijnwalraven]

I don't think this will work, because a FormData instance is itself a JavaScript object. So if we want to treat form data specially, we'll probably need a dependency on form-data.

I'd like to avoid a dependency on form-data if at all possible however, because that limits us to executing on Node. And we want Apollo Server to also run on non-Node edge environments like Cloudflare Workers and fly.io. That's the reason we don't use node-fetch directly but export it from apollo-server-env. Ideally we'd do something similar for form data. It seems form-data doesn't respect the standard FormData API though.

What is it you're trying to do here exactly? What kind of request body does your server need?

If you only require x-www-form-urlencoded, you might be able to pass in URLSearchParams instead of FormData (we'd also need to add support for that, it wouldn't work right now).

[heralden]

It should work. I tested this in the Node REPL. (I also commited a test for it)

> const FormData = require('form-data')
undefined
> let form = new FormData()
undefined
> form.constructor === FormData
true
> form.constructor !== Object
true

The constructor property returns a reference to the function (or ES6 class) that was used to create the instance. Maybe you're thinking of the typeof operator, which would always return "object" in these cases?

Our server expects a form-data as the request body, ie. one created like const form = new FormData(); form.append('foo', 'bar');. I could try your suggestion of using URLSearchParams, but please first let me know whether the constructor property is still not appropriate.

[martijnwalraven]

Ah, you're absolutely right, I completely missed that. I was indeed thinking of typeof.

I still want to serialize objects that have a toJSON method to JSON, but I'll add an extra check for that.

[BridgeAR]

This approach has caused multiple issues due to ignoring objects with a null prototype. This is done in here and many people complained about the result.

The solution would be to check for the header. No multipart header should be stringified and that way it's possible to distinguish these easily.

I opened #5070 to fix that. @uosl the solution there is to detect the content-type and to set that up front. That should accommodate your use case, right?

[heralden]

@BridgeAR I apologise for the anguish my change may have caused.

My use case doesn't exist anymore as I no longer maintain the code using it, and it's likely been changed since. Please feel free to work out a better approach! Checking the content-type header definitely sounds like a better solution.