facebook / relay

Relay is a JavaScript framework for building data-driven React applications.
https://relay.dev
MIT License
18.33k stars 1.82k forks source link

Relay keeps re-requesting fragment even if it already has it given Array[Object{key:Object=null}] structure #1325

Closed yangit closed 7 years ago

yangit commented 8 years ago

Here is minimum repo to reproduce the bug https://github.com/yangit/relay-bug It is easy to run, and based on official starwars repo

Essentially if you have object on backend

user = {
  name: 'Tom',
  friends: [
    {
      name: 'Michael',
      body: {
        weight: 78,
      },
    },
  ],
}

and Schema


const bodyType = new GraphQLObjectType({
  name: 'body',
  fields: {
    weight: { type: GraphQLString },
  },
})

const friendType = new GraphQLObjectType({
  name: 'friend',
  fields: () => ({
    name: { type: GraphQLString },
    body: { type: bodyType },
  }),
})

const userType = new GraphQLObjectType({
  name: 'user',
  fields: () => ({
    name: { type: GraphQLString },
    friends: { type: new GraphQLList(friendType) },
  }),
})

const queryType = new GraphQLObjectType({
  name: 'RootQuery',
  fields: () => ({
    user: { type: userType, resolve: () => user },
  }),
})

And field body on any of the friend will be null i.e.

{
  name: 'Tom',
  friends: [
    {
      name: 'Michael',
      body: null, //                <== here
    },
  ],
}

Than relay will never be happy about it's cache and will keep re requesting that fragment again and again every time you enter that route.

To reproduce: Run provided example and open network tab. Every time you enter route "Two" you will see new GraphQL request being sent. Expected behavior is only request data on first enter.

eugenehp commented 8 years ago

@yangit Relay caching is based on IDs of each fragment, it's generated by Relay based on the data that you have inside the fragment. The best way to avoid this would be to:

@steveluscher can you please advise us on this?

Best, Eugene

yangit commented 8 years ago

Yes, most of my objects implement Node interface with proper Ids etc. However some of them are deeply nested. And that friend node is actually a deeply nested property on an object which does implement Node interface. It is completely pointless to implement ID for all it's properties. They are meaningless w\o root object and are not exposed at API. Also GraphQL as a schema allows nullable fields. At least it should :)

eugenehp commented 8 years ago

Sounds interesting. Let me try out your sample repo, I'm sure there is a solution for this where we can all agree on. Maybe we can do a PR for this thing :D @yangit

josephsavona commented 8 years ago

Relay caching is based on IDs of each fragment

@eugenehp Not quite - caching is based on the id of records, not queries/fragments.

Than relay will never be happy about it's cache and will keep re requesting that fragment again and again every time you enter that route.

@yangit In your example, do the friends items have ids (implement Node)? If so, Relay would fetch missing any missing data per record. It sounds like the entire list is being refetched, though, which indicates that there is no id field. Without some identifier for the items in a list, Relay has no choice but to refetch the entire list if it thinks any data is missing (I suspect because of the null fields). I would highly recommend implementing the Node interface for the friends type.

yangit commented 8 years ago

I can't implement Node for friends because it is not friends.

User {
   preferences : [
      {
         sport: { //  <- this can be null
            name: "soccer"  // <- if this is null we are in trouble
         }
      }
   ]
}

Why would preferences implement Node interface? They only make sense together with user. Also what is odd. If immediate children of an array do not have IDs they are tracked just fine.

wincent commented 7 years ago

Closing out some old issues reported against the old Relay Classic API. As all future development will take place against the Relay Modern APIs, please file new issues against that. Thanks for the discussion!