Serialize instances of classes

[quentinus95]

In my app, I use TypeScript & instances of classes.

class Test { }
const lol = new Test()
lol instanceof Object
=> true
lol.constructor.name === "Object"
=> false
const toast = {}
toast.constructor === Object
=> true

[quentinus95]

@abernix is there anything I can do to get this merged?

[abernix]

It's not clear to me what this PR is for or why changing the expectations of the tests would be appropriate.

To be honest, I'm having a hard time understanding what the intention of this PR is, particularly since it's subject is suggesting that we should stringify something, but the changes here do nothing to stringify anything.

Before we can merge this, can you please put together a reproduction for the problem you're running into and update this PR with a link to that reproduction? Ultimately, that reproduction should be used to create a new test (rather than changing an old test to meet your new requirements), or you should provide evidence as to why the old test is wrong.

Thanks!

[abernix]

Why are you reversing what these two expectations are testing for?

[quentinus95]

@abernix see my comment below.

[quentinus95]

@abernix Sorry for the lack of details. Here is the situation.

The implementation of the fetch method of Apollo serializes (using JSON.stringify) objects when posting / deleting / puting... data.

Current implementation uses options.body.constructor === Object to ensure we are handling objects, and consequently are able to serialize them. The main issue with this condition is that when you're using ES6 class instances, the constructor name is the name of the class, not Object (see my previous message).

Consequently, the test I am reversing the expectations is also wrong, when passing an "instance of class", here a FormData, it should be serialized and Content-Type should be set to application/json.

I see two possibilities here:

• the test name is incorrect, and by does not serialize a request body that is not an object we mean does not serialize FormData as it is a specific use case where we don't want instances of this specific class to be serialized
• the test itself is incorrect, and we should serialize class instances (as there is no specific reason why we should not)

The correction in this pull request, is to use instanceof instead of a ===, so any class instance (which by definition in an instance of Object) will be serialized as well.

Hope this is clearer. Please tell me if you need any more information / change from my side.

[abernix]

I appreciate the clear explanation; Thank you!

I think that switching to instanceof Object is likely too much, particularly since basically everything in JavaScript is an instance of an Object (e.g. a function is; ((() => {}) instanceof Object) === true). More importantly though, this would seem to break the original use-case which introduced this code in #1316.

Perhaps @uosl could provide their thoughts?

[quentinus95]

This is a little bit sad, it would imply using using Object.assign({}, myInstance) or { ...myInstance } each time we need to call this method...

[heralden]

I agree with this PR. What @quentinus95 is saying is correct, and the test I made is semantically wrong. However, this commit will break code that depends on FormData not being serialized (which is definitely the case for our use of apollo-server), so how about accompanying it with a commit to correctly handle FormData?

I will try to look into whether there is an accurate way of identifying form data without pulling in the form-data package as a dependency. @martijnwalraven suggestion in #1316 of using URLSearchParams instead could also be an alternative solution (although this would also be a breaking change, since code using FormData would have to be rewritten to use it instead).

A different way to solve this could be to require custom classes to implement a toJSON method. What do you people think about this?

[abernix]

so how about accompanying it with a commit to correctly handle FormData?
I will try to look into whether there is an accurate way of identifying form data without pulling in the form-data package as a dependency

I don't think we should put an exception in here for form-data, even if you can find a way to determine that it is form-data-like.

The correct thing to leverage is the behavior that would be expected from JSON.stringify which is well defined. Part of that expectation includes, as @usol suggests, respecting the presence of (or lack thereof) a toJSON implementation. I'm not sure what form-data implements, but I'd like to move in that direction.

[heralden]

Alright, so a simple solution would be to check for the presence of toJSON (perhaps typeof myClassInstance.toJSON === 'function') and in that case run JSON.stringify on it. This works with form-data as it doesn't define toJSON.

If you wanted the standard object stringification for class instances, you would simply define the method to return this.

toJSON() {
  return this;
}

[quentinus95]

I don't really like having to pollute dozen of classes with a toJSON method, with no interface implementation (due to JS limitations).

Don't we have too much logic in this class?

In my opinion, it would be clearer to ask for the user to set the proper Content-Type header to get FormData handled properly, otherwise it would default to application/json.

For instance, this extract of the logic looks strange to me:

    options.body = JSON.stringify(options.body);
      // If Content-Type header has not been previously set, set to application/json
      if (!options.headers.get('Content-Type')) {
        options.headers.set('Content-Type', 'application/json');
      }

We could have a json stringified body while not getting the header to be set properly. A condition on the headers in the first place would allow to determine if we should json-encode data.

contentType := headers["Content-Type"] || "application/json"
if (contentType === "application/json")
    // set body to json encoded passed body
else
    // do nothing

[erikzillow]

I've found that using args as provided (e.g. args.userInput which is a complex object) will not serialize as-is. This seems to be because graphql uses Object.create(null) when executing, which yields args.userInput.constructor === undefined which would cause this implementation to fail.

A safer equivalent alternative would be typeof options.body === "object"

Related to apollographql/datasource-rest#68