Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Posts: enable polling with leading fetch #3136

Merged
merged 1 commit into from
Feb 8, 2016

Conversation

rralian
Copy link
Contributor

@rralian rralian commented Feb 6, 2016

I tried this in #2965 but I had to revert before it hit production. It worked fine on mounting, for pagination, and for polling queries, but I had removed the mechanism by which the component triggered a new query with different props. Whoops.

So now I'm using a leading polling query, but I've added some protection against double-fetching. The check in fetchNextPage isn't strictly necessary for my purposes, but seems like a good protection to have in general.

/cc @gwwar

I tried this in #2965 but I had to back it out before it hit production. It worked fine on mounting and for pagination, but I had removed the mechanism by which the component triggered a new query with different props. Woops.

So now I'm using a leading polling query, but I've added some protection against double-fetching on the fetchNextPage method.
@rralian rralian added Posts [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 6, 2016
@rralian rralian self-assigned this Feb 6, 2016
@rralian rralian added this to the Calypso Core: Offline 3 milestone Feb 6, 2016
@rralian rralian changed the title use polling to enable fetching when window (re)gains focus Posts: enable polling with leading fetch Feb 6, 2016
@retrofox
Copy link
Contributor

retrofox commented Feb 8, 2016

LGTM.

Just I wonder if here we have a dangerous condition when we're setting the flag used to return the value of postListStore.isFetchingNextPage() method.

Its value by default is false. When we start to fetch data this flag is set to true in the FETCH_NEXT_POSTS_PAGE action.

When the response is here the flag is set to false but it also depends of a if condition above (L135).

TBH I think you've already aware of this situation. It's just my two cents.

@rralian
Copy link
Contributor Author

rralian commented Feb 8, 2016

Thanks for the review @retrofox.

I think the code you point out is correct. The isFetchingNextPage() method only checks whether we are fetchingNextPage of the _activeList. That check makes sure that a fetch response for a prior list does not clear out the fetchingNextPage value of the current _activeList. E.g., let's say we fire off a fetch for one list, then immediately change to a new _activeList and fire off a request for that second list. When the first response comes back, we don't want to set _activeList.fetchingNextPage to false, because that request for the second list (which is now _activeList) is still in-flight. And therefore we would not want to repeat it.

@rralian rralian added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 8, 2016
rralian added a commit that referenced this pull request Feb 8, 2016
@rralian rralian merged commit b92e2d0 into master Feb 8, 2016
@rralian rralian deleted the update/post-list-fetch-on-mount branch February 8, 2016 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants