Apollo-client: Latest Apollo Client sends Date objects as {} and now requires .toISOString()

[the-simian]

Intended outcome:
Apollo recognizes Date() objects and prepares them thusly for GraphQL consumption, as was done in previous versions

Actual outcome:
The sending new Date() as an argument for an input (such as a filter) yet get interpreted as empty objects {} instead of the Date Type Graphql recognizes

How to reproduce the issue:
You can repro the issue by creating a GraphQL query and using something like a date range filter and passing new Date() as args for inputs. The GraphQL error will throw, stating that an expected type of Date was not received.

The fix (currently)
use new new Date.toISOString() instead of ajust new Date() object. So you need to call that on any dates you want to send with apollo-client

Versions
I'm using the latest npm-published apollo client and these plugins at these versions:

    "apollo-cache-inmemory": "1.4.0", 
    "apollo-client": "2.4.9", 
    "apollo-link": "1.2.6", 
    "apollo-link-context": "1.0.12", 
    "apollo-link-error": "1.1.5", 
    "apollo-link-http": "1.5.9", 
    "apollo-link-redux": "0.2.1", 
    "apollo-link-retry": "2.2.8", 
    "apollo-link-ws": "1.0.12",
    "react-apollo": "2.3.3",

[the-simian]

There's a possibility I opened this in the wrong place, there's an issue about this here, but the Date is from April 24th, 2017.
https://github.com/apollographql/react-apollo/issues/654

One thing I want to add is that I recently updated some dependencies and this was working with new Date() when I had these older versions. None were terribly out of date. Here is the version diffs:

    "apollo-cache-inmemory": "1.3.10",  → 1.4.0
    "apollo-client": "2.4.6",  → 2.4.9
    "apollo-link": "1.2.3", → 1.2.6
    "apollo-link-context": "1.0.9", → 1.0.12 
    "apollo-link-error": "1.1.1",  → 1.1.5
    "apollo-link-http": "1.5.5", → 1.5.9
    "apollo-link-retry": "2.2.5", → 2.2.8
    "apollo-link-ws": "1.0.9", → 1.0.12
    "react-apollo": "2.3.1", → 2.3.3

[danilobuerger]

Currently apollo-client does not support the serialising of custom scalars. Please be aware that Feature requests and non-bug related discussions are no longer managed in this repo. Feature requests should be opened in https://github.com/apollographql/apollo-feature-requests.

[the-simian]

@danilobuerger I actually believed this was a bug because this did previously serialize Date objects before. Even if it was unintended behavior beforehand, that's still a breaking change in behavior.
Did you read the report? I did not frame this as a feature request, I felt like I was pretty clear in explaining this was a change from previous behavior. I included all version changes so you could understand as well, and possibly identify the change in behavior so you could properly version.

I am fully aware there are utilities like https://www.npmjs.com/package/graphql-iso-date , and I'm happy to use that. But again - if your library 'did a thing' and it stops 'doing that thing' and its not a major semver change, that's a bug report.

Here's the semver spec if you're confused about how it works: https://semver.org/ So if you dropped serialization of Dates into iso formats (you did), that would be a major version change according to the spec.

Thanks for your reply. Great library, love using it and appreciate the effort put into maintaining it.

[danilobuerger]

Even if it was unintended behavior beforehand, that's still a breaking change in behavior.

So is every bug fix if you want to see it like that.

Did you read the report?

Yes I did.

But again - if your library 'did a thing' and it stops 'doing that thing' and its not a major semver change, that's a bug report.

Thats incorrect. If it did that thing, and that thing was considered a bug, and then that thing (a bug) was fixed, it would be a patch change, not major.

Here's the semver spec if you're confused about how it works

Thanks, how kind of you...

So if you dropped serialization of Dates into iso formats (you did)

Please provide prove of your claim by providing a minimal reproducible sample.

[the-simian]

Look, @danilobuerger I was just trying to help here. We may disagree that dropping a major piece of behavior is a breaking change, but I can assure you most engineers would think that it is. At the very least you have to charitable enough to see I was not framing this as a feature request. My team performed a minor version upgrade and a major piece of behavior was now different. It broke the app, as breaking changes do.

I guess my advice is that if you want people to help with your open source project maybe turn down the snark a bit? I didn't need to take the time to write up an issue that I suspect some others might also find but I did because I like this library, and I've met and like a lot of your colleagues on this project. I think others will make the minor version change, and probably be surprised when their app breaks. This is assuming that this lib is where the fault lies, it might be a different dependency. In any case, I was just trying to help.

Similarly, if this was a bug 'all along' I didn't' see any documentation about dropped support, and it has been acting the same way on all previous versions as far as I could tell. If this is a 'hey yeah that was broken all along and we fixed that' sure, cool. But I mean level with me. I made this issue so anyone experiencing the same thing would hopefully see what was up, and find the fix.

Good luck with your project, I was earnestly trying to be helpful and my honest advice is to try and be more charitable to future reports. That person you're snarking might take the time to work on a PR that's very helpful for you, which I would have gladly done, at least before today.

[danilobuerger]

snarking

Please take a look at your own response first.

I was earnestly trying to be helpful

Same here. So please provide a minimal reproducible sample. If its reproducible, it can be fixed. And I am not asking for one just for the sake of it, but because I actually did read and tried to reproduce your report and couldn't reproduce it.

[the-simian]

At the very least, I would have expected something in the tool-chain to throw an error related to an unrecognized scalar type, as I've seen before, but this was stripped of properties and turned into an object with no data therein. I'm not sure, is that also desired behavior? Maybe yes, and you do this optimistically, or rely on a plugin? I guess I expected a reply addressing that.

To your second point, I don't feel like I was being snarky, I was trying to explain why I felt that this change was more considered a bug report (even if it was a change from what-was-a-bug, it is a pretty important Scalar, right?). Especially the fact there's not really any clear errors thrown, but this might be the wrong repo to discuss that and not this lib's responsibility. That's fine also. As I said, I think even the least charitable reading of the initial report can see I'm not requesting extended functionality, but describing what appeared to be a change in behavior. Therefore, I don't feel like you were being earnest when you closed this and categorized this as a feature request. As you say, If you had tried to reproduce it in any way, you didn't communicate that to me at any point. I actually would love to have heard perhaps you telling me that even on old versions it shouldn't have done the serialization. that might have been really useful information. Really anything to help me understand what I had observed

But we're splitting hairs.

I mean, it sounds like needing the .toISOString() is, in fact, desired behavior? I was leaning on a bug before, right? Good luck with you and your team's project, and I mean that sincerely. I want this project to be good., I use it. Just to reiterate, I'm not requesting Date support as a feature, as I said; therefore, I don't feel like I have any reason to interact with this repo anymore.

[danilobuerger]

new Date() serialises just fine on apollo-client 2.4.6, 2.4.9 and 2.4.12 (even if its not supported).

If you don't want to make a minimal reproducible sample, thats fine. But then we will never find out why you can't get new Date() to work, although it works with every version of apollo-client you listed above.

[the-simian]

Ok, wait so it should be serializing? I'm confused, is it non-officially supported? This is the discussion I wanted all along, seriously. I just want to know what's happening, that's it.

Edit, ok I reread your reply. It looks like 'officially' no, its not supported, but it does attempt to serialize right now. I hope you can understand this was confusing for me at least. Thanks for spelling that out. The minor update should have kept doing what it did beforehand (at least that's what it did for you in testing), right? the problem might be in another dependency, then.

[danilobuerger]

It does (not should) serialise new Date() correctly. But this is not part of the API. Whatever happens with custom scalars is undefined as they are not supported. It works with Date, just unintentional.

[the-simian]

Yes I see that this is not officially part of the GraphQl spec, but I hope you can understand what this lib should be doing and does do; well there was a little confusion here for me as an end-user. At least there was before today. I also hope you can appreciate that discovering "unintentional" behavior lends more credibility to my initial concerns I might have found a bug.

Would a PR for a warning message be useful for this? Something like 'hey Date isn't officially supported use such-and-such'? Just pitching an idea here.

Edit: Also thank you for sticking with this and talking to me, I feel like we're more on the same page now. I personally think clearly spelling out the default Date behavior is important. It's weird because even though Date isn't technically a primitive in Js or GraphQL spec I could see this being encountered often, as Date is a Native Constructor, and folks will be using it a lot. Curious on your thoughts here.

[danilobuerger]

Let me start over, I think it will become clearer what I meant by supported.

Date is a custom scalar and not part of the graphql spec. So on your graphql server you would declare it as a custom scalar:

scalar Date

And then implement functions to marshal and unmarshal it. That doesn't mean though that you are unmarshaling the date as defined per RFC 3339 (or ISO 8601). It could be

parseValue(value) {
  if (value === 'today') {
    return new Date();
  }
  return new Date(1970,1,1)
}

so totally different than you would expect. Because reasons 😅.

Now on the client, we have no way of knowing how the server actually marshals and unmarshals a custom scalar, and thus a date. Therefor, its unsupported.

That doesn't mean however, that new Date() wouldn't serialise as expected. It will. Internally JSON.stringify is used and anything you input it will be serialised according to its rules.

To recap:

• Serialising new Date() to work with the servers scalar Date is not supported.
• Serialising new Date() will result in whatever JSON.stringify does:

> JSON.stringify({ date: new Date() })
{"date":"2019-01-23T21:46:27.285Z"}

You can try this out:

import { InMemoryCache } from "apollo-cache-inmemory";
import { ApolloClient } from "apollo-client";
import { HttpLink } from "apollo-link-http";
import gql from "graphql-tag"

const client = new ApolloClient({
  cache: new InMemoryCache(),
  link: new HttpLink({
    fetch: (url, options) => {
      console.log(options.body);
      return fetch(url, options);
    }
  })
})
const TEST_MUTATION = gql`
mutation foo($date: String!) {
  bar(date: $date) {
    id
  }
}
`
client.mutate({ mutation: TEST_MUTATION, variables: { date: new Date() }})

this will log:

{"operationName":"foo","variables":{"date":"2019-01-23T21:51:29.291Z"},"query":"mutation foo($date: String!) {\n  bar(date: $date) {\n    id\n    __typename\n  }\n}\n"}

containing the correctly serialised date.

[the-simian]

Ok, what you posted there (very helpful) is what I was observing before 4.9, but after that was stringifying to {} this may not be this libs fault at all if you couldn't reproduce. I do see what you mean about being able to declare a scalar Date and it doing something else, (such as using a non-ISo or whatever). The code you posted is more-or-less exactly what we're doing with 4.9 though, and we haven't implemented any functions to tamper with default stringification.

I think I might try again with 4.12 and see what it does. However, you spelling out what apollo-client should do (call JSON.stringify on unexpected items) is useful. Something is weird there for me in 4.9, but I actually am not sure what it is yet. I am beginning to suspect it might be one of the plugins, but probably not the InMemoryCache or HttpLink because I see you're using those also.

I think we're good here, and if anyone else comes across this problem I think the thread has a lot of utility.

[loremaps]

Hi @the-simian are you using apollo-upload-client? If so, then your issue is related to this jaydenseric/apollo-upload-client#138.

[the-simian]

@loremaps yep, good catch, thank you.