Skip to content
This repository was archived by the owner on Apr 13, 2023. It is now read-only.

fetchMore does not rerender redux connected components #549

Closed
cdreier opened this issue Mar 20, 2017 · 17 comments
Closed

fetchMore does not rerender redux connected components #549

cdreier opened this issue Mar 20, 2017 · 17 comments

Comments

@cdreier
Copy link

cdreier commented Mar 20, 2017

Steps to Reproduce

Describe how to reproduce this issue.

const mapState = state => ({})
const bindDispatch = {}
const connectedList = connect(mapState, bindDispatch)(ListingComponent)

const withGqlData = graphql(GraphQLQueries.categoryDebug, {
  options: () => {
    return {
      variables: {
        pageNumber: 1,
      },
    }
  },
  props: (debug) => {
    const articles = (debug.data.category) ? debug.data.category.list : []
    return {
      testProp: 'yay!',
      list: articles,
      loadMore: () => {
        return debug.data.fetchMore({
          variables: {
            pageNumber: 2,
          },
          updateQuery: (previousResult, { fetchMoreResult }) => {
            if (!fetchMoreResult) { 
              return previousResult 
            }
            const newList = [
              ...previousResult.category.list, 
              ...fetchMoreResult.data.category.list,
            ]
            // at this point, everything is fine, the newList has correct and extended data
            console.log('MORE RESULTS, RETURN NEW LIST', newList.length)
            return Object.assign({}, previousResult, {
              list: newList,
            })
          },
        })
      },
    }
  },
})

export default withGqlData(connectedList)
  1. created redux container above (in a react native app, not tested in web environment)
  2. initial response triggers componentWillReceiveProps and connected component rerenders the data
  3. loadMore is triggered from component
  4. query response has next page data, see logs and comments
  5. nothing happens

Buggy Behavior

componentWillReceiveProps is not called after fetchMore returned new state, so nothing new is rendered though the new data is available

Expected Behavior

after fetchMore returns new data, componentWillReceiveProps is called and component rerenders

Version

  • apollo-client@1.0.0-rc.5
  • react-apollo@1.0.0-rc.3
@HorizonXP
Copy link

HorizonXP commented Mar 21, 2017

I'm actually also experiencing similar issues with fetchMore.

Steps to Reproduce

const mapState = state => ({
    currentArea: state.currentArea // initial value set from store
})
const bindDispatch = {
   setArea: (payload) => ({ type: 'SET_AREA', ...payload })
}
const connectedList = connect(mapState, bindDispatch)(ListingComponent)
const withGqlData = graphql(GraphQLQueries.categoryDebug, {
  options: ({ currentArea }) => {
    return {
      variables: {
        currentArea
      },
    }
  },
  props: (debug) => {
    const articles = (debug.data.category) ? debug.data.category.list : [];
    const currentArea = debug.ownProps.currentArea;
    const oldArea = calculateArea(debug.data.category);
    return {
      testProp: 'yay!',
      list: articles,
      loadMore: () => {
        return debug.data.fetchMore({
          variables: {
            currentArea,
            oldArea
          },
          updateQuery: (previousResult, { fetchMoreResult }) => {
            if (!fetchMoreResult) {
              return previousResult
            }
            const newList = [
              ...previousResult.category.list,
              ...fetchMoreResult.data.category.list,
            ]
            return Object.assign({}, previousResult, {
              list: newList,
            })
          },
        })
      },
    }
  },
})
export default withGqlData(connectedList)
  1. created redux container above (in web environment)
  2. First page is loaded, correctly
  3. loadMore is triggered from component, after SET_AREA is fired
  4. loadMore query is fired with currentArea === (initial area), oldArea === null
  5. Instead, the original GraphQL query is fired, but with currentArea === (our newly set area), which gets the right data

Buggy Behavior

(I'm using new and old areas to describe what's really just a cursor split into two.)

So my actual code is a bit more complex, but I've rewritten it many times over in search of the issue causing this bug. Eventually, I got to the point where I ripped out Redux & connect, and just called loadMore directly with the new and old areas passed in as parameters. What I determined was that when loadMore is fired, my dataset is empty, so my area calculations obviously don't work.

Expected Behavior

I expect that when loadMore fires, it has a reference to the existing set of data. Somehow, I think this is getting garbage collected, or perhaps I'm misunderstanding the sequence of events. But based on the cursor pagination example, and the fact that I removed all Redux code except for Apollo so that nothing else was mutating state, I don't think that's the case.

Version

  • apollo-client@1.0.0-rc.5
  • react-apollo@1.0.0-rc.3

@HorizonXP
Copy link

HorizonXP commented Mar 21, 2017

As a bit of an update, I put some logging inside of loadMore, and it's only ever called with loading: true and networkStatus: 1

@helfer
Copy link
Contributor

helfer commented Mar 21, 2017

@cdreier @HorizonXP Thanks for reporting the issue. Could one of you provide a quick reproduction with https://github.com/apollographql/react-apollo-error-template? That would help us a lot in resolving this issue.

@HorizonXP
Copy link

@helfer Done: https://github.com/HorizonXP/react-apollo-error-template/tree/fetchmore-data-error-repro

That reproduction is a little convoluted, so let me provide context. In my application, I'm loading a number of points from GraphQL that I want to plot on a map. Thus, as the user moves around the map, I'm using fetchMore to get more data points from my GraphQL endpoint. However, I update my query to also send oldBounds, so that the server doesn't have to send me data points that I already have on the client. So in my application, the sequence is:

  1. Initial page loads with map bounds set to (x,y)
  2. Apollo calls my query with bounds = (x,y) variable set, and loads the appropriate data points
  3. User pans/zooms the map, and when they're "done", an event is fired, which I use to update my Redux state about their current position
  4. At the same time, I call my loadMore() function
  5. My goal is to have the query variables set to bounds = (newX, newY) and oldBounds = (calcX, calcY) where newX and newY are the new map boundaries that the user moved to, and calcX and calcY are the actual boundaries for the existing client-side dataset.
  6. Server searches whole area using bounds = (newX, newY) but only returns a subset, since the client told us they already have data from oldBounds = (calcX, calcY)
  7. Client combines the newly received dataset with the existing dataset and redraws.

I've tried to recreate something along these lines in react-apollo-error-template.

  1. Initial page loads with id = 1
  2. Apollo queries with id = 1 variable set, and loads the appropriate people data
  3. User clicks the Set Id to 6 and load more... button to fire Redux actions that update id = 6, and calls loadMore()
  4. Apollo should be calling updateQuery and fetchMore with variables set to id = 6 and lastId = 10, since we have data for persons 1 to 10.

Step 4 doesn't happen, and while producing this repro code, I may have figured out what's happening. Anyway, here's what happens:

  1. Apollo instead calls the initial query again, except this time, with id = 6. We see this on the server-side, since we log the arguments being sent. So the server sends people 6 through 15.
  2. Apollo then calls loadMore, with variables of id = 1 and lastId = 10. The server then sends an empty array back, since we've told it we already have people up to id of 10.
  3. Client-side only shows people 6 through 15.

So now I'm not convinced this is a bug in react-apollo, but rather a misunderstanding on my part about the sequence of events. In any case, the issue is that Apollo is firing a brand new query, which is why it's discarding the existing data. That's not what I want, I want to keep the existing query. I think the issue is that Redux is redrawing the React component. So in my code, what I think is happening is actually this:

  1. setId and loadMore() are both called
  2. loadMore() executes before setId has updated state, and returns the fetchMore promise using our non-updated values.
  3. Redux re-renders our component, causing Apollo to call our query with the updated values.
  4. The fetchMore promise returns, but it used stale values, and it's for a query we're no longer using, so it's useless.

I think I need to get Redux to not rerender when the state is updated. I'll need to play with this some more.

@bondz
Copy link

bondz commented Mar 21, 2017

You might want to have a look at apollo-client changelog and apollographql/apollo-client#1416

fetchMoreResult no longer puts the result in the data property, you can access it directly. So,

const newList = [
  ...previousResult.category.list,
  ...fetchMoreResult.data.category.list,
];

becomes

const newList = [
  ...previousResult.category.list,
  ...fetchMoreResult.category.list,
];

@cdreier
Copy link
Author

cdreier commented Mar 21, 2017

@bondz thank you - already noticed that and was not the problem in my app :/
@helfer i tried to reproduce the bug in the web template too, but everything is working as intended... https://github.com/cdreier/react-apollo-error-template
i will have a look tomorrow in the android app, perhaps it is special behaviour in react native?

@HorizonXP
Copy link

So I was able to get my repro to work correctly.

Works: https://github.com/HorizonXP/react-apollo-error-template/tree/dd4677801fbb032ba34519efde6598ab990c8927

Doesn't work: https://github.com/HorizonXP/react-apollo-error-template/tree/e57e38d24369bfc14904cc1a1d91de6b7c27c539

So what I figured out is that react-apollo is rerendering the component from scratch when the id property is changed. Using react-redux connect, I was able to skip that check. In a real Redux reducer, you wouldn't just return true from areStatePropsEqual, otherwise you wouldn't be able to respond to updates on other values. In the working version, the issue is also that the loadMore() promise is created before the Redux state updates. You could probably wait to call loadMore() after the Redux action promise returns, but I don't think that will work as well either.

Anyway, I'd chalk this up to some confusion about the interoperability between Apollo, Redux, and React. I'm not sure if there's an action item from this issue.

@HorizonXP
Copy link

Sadly, I can't seem to use this new knowledge to fix my actual project. I can now get it to only fire fetchMore, but every time it fires, it thinks the existing dataset is empty, making it impossible for me to calculate the new boundaries.

@HorizonXP
Copy link

Finally figured it out! The issue was that in my application, my React component was a stateless component, so the closure wasn't being recreated each time. Forcing it to be a class-based component with a stateful function defined on it solved my issue.

That took 2 days of debugging. But wow, do I have a better understanding of how this all relates. Sorry for airing this out on the issue tracker!

@rbpinheiro
Copy link

rbpinheiro commented Mar 23, 2017

I had a similar issue (no redux), and solved by altering two things in my code.

My "updateQuery" method would concat an array altering a property on the previous result and return that.
Now I create a new object from scratch:
return { ...previousResult, cover: { ...previousResult.cover, cards: previousResult.cover.cards.concat(newCover.cards) } };

I was exporting my component like this:

export default withData(MyComponent);

Now I assign that to a constant before exporting:

`const MyComponentWithData = graphql(CONTENT_QUERY, {
(options and props)
})(MyComponent);

export default MyComponentWithData;`

Not sure why it solves the issue, but maybe it will help someone out.

@yury-dymov
Copy link

yury-dymov commented Apr 19, 2017

Facing the same issue.

I played around with different versions and figured out that with apollo-client v0.9.0 and below this issue is not happening regardless of react-apollo version

@helfer
Copy link
Contributor

helfer commented Apr 21, 2017

@cdreier @yury-dymov I now think this has to do with the fragment matcher. Could you try the solution outlined in the link below and report back, please?

apollographql/apollo-client#1555 (comment)

@witbybit
Copy link

@rbpinheiro Thanks a lot! I was struggling with this issue for more than a day before I came across your solution. No idea why this solves the problem though.

@jbaxleyiii
Copy link
Contributor

looks like a solution was found?

@esperancaJS
Copy link

esperancaJS commented Oct 26, 2017

@jbaxleyiii no, this should be re-open

@esperancaJS
Copy link

esperancaJS commented Oct 26, 2017

I believe I have found a cause/symptom of the problem in my case: adding a new attribute to the returned object.

so it the object is:

{
   id: 100,
   a: 1
}

So, this doesn't work:

// (...)
updateQuery: (previousResult, { fetchMoreResult }) => {
   return {
      id: 100,
      a: 1,
      b: 2 // new
   }
}

But this does:

// (...)
updateQuery: (previousResult, { fetchMoreResult }) => {
   return {
      id: 100,
      a: 2, // different
   }
}

Also, this returns the object in render() without b:

// (...)
updateQuery: (previousResult, { fetchMoreResult }) => {
   return {
      id: 100,
      a: 2, // different
      b: 2  // new
   }
}

@tommymarshall
Copy link

tommymarshall commented Jan 26, 2018

Has this issue been resolved? Running into this now and semi-losing my mind :)

Resolved: I did not realize that this simply grabbed the data from the server, but does not pass it as props to the component directly -- it still calls the graphql and is sent to the props function.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants