Make it easier to ensure the right fields are present for dataIdFromObject

[dahjelle]

Idea: should Apollo print a warning if dataIdFromObject is provided, but returns something falsey? Apollo currently uses it's default itemDataId or valueDataId if the response is falsey, but it wasn't immediately clear to me why my queries suddenly started breaking.

Happy to submit a PR if this sounds like a good idea. Would throwing an error or just logging a warning to the console be preferable?

---

My app had 2 queries:

A

{
  person(id: "5") {
    id
    prop_one
    prop_two
  }
}

B

{
  person(id: "5") {
    # NOTE: I forgot to include `id` here, so `dataIdFromObject` will return `undefined`
    prop_three {
      id
      prop_four
    }
  }
}

I also specified dataIdFromObject as

export default function dataIdFromObject(obj) {
  return obj.id;
}

---

Query A ran fine and stored data in the store. Query B executed later, but, since dataIdFromObject returned undefined (see the note in B), the store now contained

"ROOT_QUERY": {
    "people_one({"id":"5"})": "ROOT_QUERY.people_one({\"id\":\"5\"})"
}

instead of

 "ROOT_QUERY": {
    "people_one({"id":"5"})": "5"
}

which broke retrieving data for query A.

[abhiaiyer91]

Ouch. Yes I think more warnings the better!

[stubailo]

Totally agree, having a warning for this would be great.

Since this function might be called in several places, it might be best to just wrap it where it is passed in: https://github.com/apollostack/apollo-client/blob/82aa1aceaac589926479000fdd872cbe063c8a73/src/index.ts#L177

[Poincare]

What about the case where we have a query that doesn't include an id? In that circumstance, we would expect dataIdFromObject to return a falsey value, right?

[dahjelle]

It's almost as if, before writing to the store, one wants to look at the existing data, and if it doesn't "look" like the default key (i.e. ROOT_QUERY._), then warn if you are about to write over it? I don't know enough about the store format to know if that really makes any sense.

For instance, I see there are quite a few tests in mutationResults.ts that have a dataIdFromObject that intentionally is returning null. Similarly, if one had a numeric ID of 0, I guess that would be falsy, too, and I think that breaks several things.

(What's the proper spelling of 'falsy' anyway? falsy? falsey?)

[dahjelle]

I started a potential change but I don't think this is the best design, if only for all the warnings printed in the mutationResults tests.

[Poincare]

That's exactly what I meant in my previous comment. I don't think warning the user every time that there is a falsey value returned is a great approach because this seems like a perfectly reasonable thing to do if we don't have an id for a particular object.

[stubailo]

Yeah that's true - I think I misjudged the situation.

We should see if there is a way to warn people when they didn't have an id but intended to have one, but it's totally reasonable for not all returned objects to have IDs!

[dahjelle]

We should see if there is a way to warn people when they didn't have an id but intended to have one, but it's totally reasonable for not all returned objects to have IDs!

Agreed.

I'm not sure I see a good way of doing that, though. Check if dataIdFromObject returns undefined?

Looking at this a different way, my issue was entirely due to a misshapen query: it didn't match what dataIdFromObject was expecting. Is there another way to define dataIdFromObject (i.e. a name of a field? a fragment?) such that one could easily compare against the query to see if they are compatible?

In my case, I'm not sure that this is a high priority for me any longer: now that I know what I'm looking for, it's not too hard to figure out the problem. On the flip side, for new users of Apollo, it's a pretty subtle and confusing problem without a good warning.

[stubailo]

Yeah, this is definitely one downside of the current approach - that you need to manually make sure the id is in your queries. I don't have a good solution at the moment, but we should think about it more for the future. One option is to figure out some way to automatically add IDs to the query.

[saikat]

I have an implementation of dataIdFromObject that does this:

dataIdFromObject: (result) => {
    if (result.hasOwnProperty('id')) {
      return result.id
    }
    return randomString()
  }

However, if I forget to include id in my queries, I still get this error:

Error: Can't find field displayName on object [object Object].

On a query like this:

    query: gql`{
      currentUser {
        displayName
        email
      }
    }`

Is this to be expected? I was hoping that returning a randomString would just duplicate the object in the client-side store, but not lead to any actual problems.

[Poincare]

If you return a random string and this string is different every time that the we have to get the object from the server, that will cause problems in looking up the element when we want to read from the store again (i.e. the dataIdFromObject must be a pure function). Instead, you should just return "nothing" (i.e. undefined) and Apollo Client will take care of generating the id for the object.

Is there a particular reason why you would like to duplicate the object within the store?

[saikat]

Nope, it just wasn't clear to me how to invoke the default action that
Apollo Client would take from this method.

On Sat, Jul 23, 2016 at 4:40 PM Dhaivat Pandya notifications@github.com
wrote:

If you return a random string and this string is different every time that
the we have to get the object from the server, that will cause problems in
looking up the element when we want to read from the store again (i.e. the
dataIdFromObject must be a pure function). Instead, you should just
return "nothing" (i.e. undefined) and Apollo Client will take care of
generating the id for the object.

Is there a particular reason why you would like to duplicate the object
within the store?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#380 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAFHRtRxZgcCPRZQV8W_BBHq9pJGeCqnks5qYnxRgaJpZM4JJZeI
.

[aweary]

@saikat you can see the behavior in writeToStore

[joarwilk]

Do you have access to the GQL schema definition inside the client? A good case would be to log warnings when the id field is present in the schema but not in the query. I have some types without an ID and for those it'd be nice to not warn even though dataIdFromObject may return falsy.

Expanding on that case, a client setting which auto-includes the id field for all types which has an id in the schema would be very useful.

[stubailo]

Right now we are trying not to require having the schema on the client or in a build tool, but I'd definitely be open to something optional that people can use if they want to. There are a few options:

1. Load the schema on the client only in development, to provide the warnings mentioned above
2. Load the schema in a developer tool like an ESLint plugin, that warns you when you are missing the ID field
3. Load the schema in a build plugin, which precompiles queries in your code and includes these fields for you

Thoughts?

[joarwilk]

Having it as an ESLint plugin feels like it makes the most sense imo since it'd hook into most users regular workflow and you can add it on an on-demand basis.

[stubailo]

We could integrate it as a new rule into this project: https://github.com/apollostack/eslint-plugin-graphql

[helfer]

I'm going to close this since it seems that the consensus is not to do anything about this in apollo-client, but rather do it via linting. Once __id becomes a thing, we can open a new issue / PR.

[stubailo]

This is also a good candidate for the new Apollo devtools we are working on.

[spacebeers]

@helfer "Once __id becomes a thing". Could you elaborate on this please?

[stubailo]

@spacebeers unfortunately there hasn't been much movement in this area but back in the day we were hoping that there would be a graphql spec standard for an ID field similar to __typename that could be injected.