WIP: Return undefined for fields not in store. (fixes #359)

[dahjelle]

I'm still experimenting with this in my app, so I'm not sure this is at all the correct approach to fix #359. Any thoughts/pointers/direction?

TODO:

• Update CHANGELOG.md with your change
• Make sure all of the significant new logic is covered by tests
• Rebase your changes on master so that they can be merged easily
• Make sure all tests and linter rules pass

[stubailo]

Hmm, what's better - returning null or undefined? GraphQL servers return null in this case, so maybe it's better to match that behavior?

[Pajn]

undefined feels more javascripty and avoids the pitiful typeof null === 'object'. Consistency might be a good point, but not being consistent lets you differentiate between not in store and null from server but encouraging that logic by differentiating null and undefined might be bad as well..

[stubailo]

Also, I don't understand why this change actually fixes the issue you're talking about - I don't see how attaching an undefined property results in more data being returned somewhere else.

[dahjelle]

null vs undefined

I'm not sure, either. undefined is inconsistent with GraphQL and JSON, but lets you determine (for instance) when you need to refetch from the server. Perhaps doing something like returning null but having a partialData flag of some sort so the client could know when it is possible the server might have more data? But I've not looked at how that would get passed around…

Fixing #359

@stubailo The existing code just returns an empty object when any field in a selection set isn't available in the store. My change would at least add the field names with undefined/null attached to them. In other words, it's not attaching undefined that helps as much as not throwing away the (sic) key/field name.

[stubailo]

Hmm, I guess if it's undefined now you need to do typeof field === 'undefined' everywhere? Would a special flag be better?

And I'm going to need to look deeper at that issue, I'm not completely clear on what the issue is - I would have thought it would return all of the fields except the missing ones, but you're saying it just throws away everything if there is one missing field?

[dahjelle]

Right, it throws away everything if there is a missing field. I would have loved for it to return all fields except the missing one—that's the behavior I expected, too! (Now that you mention it…I'll look and see if there is an approach more like that possible. I suppose the disadvantage then is that you can't guarantee the "shape" of the response.)

[stubailo]

@dahjelle yeah I definitely want to return all the fields we have. If you could add some tests for that it would be a great start, then maybe I can help implement the fix.

[dahjelle]

@stubailo Just to make sure I'm on the same page, you are thinking a failing test or two on diffSelectionSetAgainstStore showing what fields ought to be returned, correct?

[dahjelle]

Pushed up some tests + a possible change. Before the change,

const { result } = diffSelectionSetAgainstStore({
  store,
  rootId: 'ROOT_QUERY',
  selectionSet: getQueryDefinition(queryWithMissingField).selectionSet,
  variables: null,
  throwOnMissingField: false,
}); 

would set result to {}. With the change (and what the test checks for) is that result is

{
  people_one: {
    name: 'Luke Skywalker',
  },
}

Look at the tests for the details of the query and what is in the local store at the time.

@stubailo Is this more along the lines of what you were thinking?

[stubailo]

Yeah this is what I was thinking! I think the original behavior was a bug, my intention was always to return as much data as possible.

[stubailo]

Looks like you fixed it as well, does this address your original issue?

[dahjelle]

@stubailo Yes, this completely addressed my original issue, too. If this looks good, I'll update the changelog…anything else necessary to get it merged?

[stubailo]

Nope looks great to me! We can merge and release ASAP

[dahjelle]

@stubailo Awesome! Thanks! There's the updated CHANGELOG.