Use getNonEmptyObj in apolloDataToVariant

[Schmavery]

Match the behavior of convertJsInputToReason and don't run the parse function
when the data object is empty. This prevents runtime crashes of the parse function
in this case. This seems like a safe thing to do because it makes the handling of
apolloData.data more consistent between apolloDataToVariant and convertJsInputToReason.

More context/explanation:

I'm no graphql expert, so I've had some trouble understanding exactly why
this is happening and build a small reproducible example, but this seems to be a fix that
makes sense and is working in our code.

I've been getting errors similar to the following when multiple different queries include the same field (in this case, "ownedWallets").

js_exn.js:6 Uncaught Error: graphql_ppx: Field ownedWallets on type Query is missing
    at Object.raiseError (js_exn.js:6)
    at parse (SideBar.js:123)
    at Object._1 (curry.js:65)
    at apolloDataToVariant (ReasonApolloQuery.bs.js:32)
    at convertJsInputToReason (ReasonApolloQuery.bs.js:57)
    at Object.children (ReasonApolloQuery.bs.js:125)
    at finish (react-apollo.browser.umd.js:470)
    at Query.render (react-apollo.browser.umd.js:474)
    at finishClassComponent (react-dom.development.js:14741)
    at updateClassComponent (react-dom.development.js:14696)

When inspecting the input to convertJsInputToReason , it's getting an apolloData object with a data field that is just an empty object (even though loading is false and there are no errors).
This gets passed to apolloDataToVariant, which tries to run the parse function (generated by graphql_ppx) because apolloData.data is not null (it's {}). parse then crashes with the above error because none of the required fields are in the object, obviously.

From my inexpert debugging, the root cause of the empty data seems to be caused by
something caching-related cause it doesn't seem to happen with caching turned off,
but this fix stops the crashing, and renders with an error "No data" in this case.

When combined with partialRefetch=true, there is no crash and no error and life is great :)

/label bugfix

[apollo-cla]

@Schmavery: 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/

[Schmavery]

(I signed the agreement)

[Gregoirevda]

Thanks for the PR, totally makes sense, but I need to do more tests to understand why this never happened before.

The runtime error Field ownedWallets on type Query is missing looks like your graphql_schema.json contains the ownedWallets field in all your queries, but your GraphQL schema doesn't. Could you try to run yarn send-introspection-query YOUR_ENDPOINT to make sure the schema is up to date. This would change the runtime error into a compile error.

Could you also try, if not added, to add an id field in all queries containing ownedWallets. This is to be sure normalisation goes well.

[Schmavery]

Makes sense, thanks for the ideas. Let me try to give some more context for what I've tried and what's happening.

My first thought was also an out-of-date graphql_schema.json, so I made sure that was up to date and in fact went to the graphiql interface of our backend and made the same query there to ensure it was working (it was). In fact, we were already querying this field before I made the change to our code that started triggering the bug.

More specifically, the scenario is something like this:
We have one query in our sidebar like this

query getWallets {
  ownedWallets {some fields}
}

I then added a button that, when clicked, opens a view next to the sidebar that queries ownedWallets as well as some other data:

query getSettings {
  version
  ownedWallets {some fields}
}

After adding this, clicking the button would unpredictably crash the whole app with the error mentioned in the description of the PR. Strangely, as you can see in the stacktrace, it wasn't the new query that was crashing, it was the old one (in the sidebar).

I tried adding the id (after reading some blog post about Apollo caching issues that mentioned it) but haven't had luck there yet and figured it didn't make much sense for a valid query to cause runtime crashes (even if it has suboptimal caching behaviour) so I wanted to see if I could fix that part at least. I will have to read more about normalization (thanks for the link) and experiment more with our schema going forward but unfortunately ran out of time to work on this bug for now.

If I get the time I'll try to formulate this into a standalone repro but can't spare the time right now so I was hoping this little bugfix could just sneak in in the meantime haha 😛

[Gregoirevda]

I added a client/server example in the examples directory:
https://github.com/apollographql/reason-apollo/tree/master/examples/pr/196

I couldn't reproduce the runtime error.
One thing about the ppx is that it only executes when you change the query, so you can get in an strange state: mhallin/graphql_ppx#41

Could you try to change your query save and get back to the original... (Who knows :D )

[Schmavery]

Nice example, interesting that it doesn't repro, that's too bad. Not sure what's up with that.

Right now I don't think it's a graphql_ppx problem, because while debugging, I went into the generated JS code, it looked right (of course I also bsb -make-cleaned and rebuilt everything, regenerated the graphql schema, all the standard stuff) and then I printed the value returned by apollo before it ever goes into the ppx, and it was an empty object. I got the feeling that no matter what the parse function was, it would pretty much always crash on an empty object :P

Looking through the internet, I found a lot of people talking about react-apollo returning empty objects in some edge cases. (apollographql/react-apollo#2114, apollographql/react-apollo#2925, apollographql/react-apollo#2370). I definitely don't have expertise on the apollo internals/caching, so I can't fully understand what exactly causes this and maybe some of those links aren't relevant to this situation, but it seems like if any of these were to happen when running reason-apollo right now, it would result in this crash. Either this is a standard graphql thing that graphql_ppx needs to handle somehow, or it seems like reason-apollo shouldn't be attempting to run the parse function in this case?

[Gregoirevda]

I also saw some empty data object while error and loading are false issues.
Thank you for this PR! Will ping you when published

[Gregoirevda]

@Schmavery Just published under 0.16.2