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: ensure a fetch on mounting for post-list-fetcher #2965

Merged
merged 1 commit into from
Feb 5, 2016

Conversation

rralian
Copy link
Contributor

@rralian rralian commented Feb 1, 2016

This should address stale data that can otherwise be shown in the desktop app. I don't think this should have much of an impact on server load, like decreasing the polling interval would.

@blowery what do you think of this? I'm really not sure why we didn't want post-list-fetcher to trigger a fetch on mounting in the first place. Anything in the reader that you'd be concerned about?

Testing

I was recreating a reported issue in the desktop app by doing the following in desktop and calypso master.

  • open the desktop app, navigate to posts-list page
  • in another browser change the title of the top post
  • click to the reader, click back to the post-list page
  • note that the post-list page can display outdated title for up to a full minute

Then by running the desktop app with this branch for calypso, if you do the same steps as above, you will still see the old title for a split-second, but then it should update to reflect the new title.

@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 1, 2016
@rralian rralian self-assigned this Feb 1, 2016
@rralian
Copy link
Contributor Author

rralian commented Feb 1, 2016

Here's a video showing posts-list getting updated right away when reopened (using this branch in the desktop). https://cloudup.com/c_5u5syesQk

@@ -126,7 +126,7 @@ PostListFetcher = React.createClass( {

componentDidMount: function() {
var postListStore = postListStoreFactory( this.props.postListStoreId );
this._poller = pollers.add( postListStore, actions.fetchUpdated, { interval: 60000, leading: false } );
this._poller = pollers.add( postListStore, actions.fetchUpdated, { interval: 60000, leading: true } );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Omitting the attribute should set it to true as well.

@mtias
Copy link
Member

mtias commented Feb 2, 2016

This makes sense to me. I believe we may have had a situation where the controller initiated a fetch at the start, and the poller would run another one immediately. For what is worth, leading was set to false since the start here.

@rralian
Copy link
Contributor Author

rralian commented Feb 2, 2016

I believe we may have had a situation where the controller initiated a fetch at the start, and the poller would run another one immediately.

Yes, there is definitely a double-fetch happening when you first load. I'll see if I can figure it out.

@mtias
Copy link
Member

mtias commented Feb 2, 2016

I wonder if we could increase the polling interval as well.

@blowery
Copy link
Contributor

blowery commented Feb 2, 2016

@rralian we don't use the post-list-fetcher in the reader. :) we handle kicking off the initial fetch up in the controller. I've been meaning to move to post-list-fetcher, but it's never bubbled up and gotten done.

@rralian rralian force-pushed the update/post-list-fetch-on-mount branch from 9528971 to 151bb2b Compare February 2, 2016 21:37
@rralian
Copy link
Contributor Author

rralian commented Feb 2, 2016

The double-fetch was happening because we were using some pagination logic to trigger the first fetch. I just removed that in my latest commit and it's working fine for me.

@rralian we don't use the post-list-fetcher in the reader

@blowery cool, thanks for checking in.

I wonder if we could increase the polling interval as well.

@mtias Maybe, but I wouldn't do so here. If we want to tweak, I think we also need to have some idea of what effect we are trying to create, and then track against a metric to see if the tweak is a success.

Testing

We use post-list-fetcher on these pages:

http://calypso.localhost:3000/posts/
http://calypso.localhost:3000/pages/
http://calypso.localhost:3000/post (post editor... the draft drawer and the post-scheduler popover)

You can test this by watching the network tab and filtering on "posts" to see that only a single fetch happens initially, and then polling fetches happen every minute. And of course you'll see pagination fetches as necessary if you scroll.

Also test to make sure the we're not screwing up pagination. The list of posts when paginated (scrolling past 20 posts) should be the same in this branch that you would see in wpcalypso.

You can also test that this helps pages reflect recent changes. Open the posts-list page. Then edit the title of one of the posts in another tab or window. Then move back to the posts-list page. You should see the new title reflected after a split-second. In master you could be waiting up to a minute to see the change.

@rralian
Copy link
Contributor Author

rralian commented Feb 2, 2016

oh, btw @blowery... I looked around and the only other instance where we use poller with an option of { leading: false } is in feed-streams.js. You may want to give that a look to see if you can change it to use a leading fetch, to avoid the stale data issue we had in the post-list-fetcher.

@mtias
Copy link
Member

mtias commented Feb 3, 2016

👍

@mtias mtias 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 3, 2016
@blowery
Copy link
Contributor

blowery commented Feb 3, 2016

Thanks for the heads up @rralian, I'll check into it.

@rralian
Copy link
Contributor Author

rralian commented Feb 3, 2016

argh... I noticed a double-fetch on the /posts page with sites that have < 20 posts (probably coming from infinite-scroll logic). need to track that down. also the draft drawer usually works for me, but occasionally it does this:

blank drafts drawer

It appears to be some sort of rendering issue in the browser, as when I inspect the items they contain the correct html. And if I toggle a css rule from the inspector, it fixes it. If I can confirm this sometimes happens in master, I won't consider it a blocker here. In any case though, back to digging a bit more.

@rralian rralian force-pushed the update/post-list-fetch-on-mount branch from 151bb2b to 4e8ae19 Compare February 3, 2016 20:57
@rralian rralian added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Feb 4, 2016
@rralian
Copy link
Contributor Author

rralian commented Feb 4, 2016

ok, figured it out. Previously when post-list-fetcher was mounted and we had no Posts data, we ran fetchNextPage() immediately to pull the first page of Posts for the query. However, my changes were using the polling method fetchUpdated to get the initial list of posts. Those two methods behave slightly differently. And that was causing some little issues.

So I'm still using polling to initiate fetchUpdated() when the component mounts (or (re)gains focus), but if we have no posts, we exit out of fetchUpdated() and instead run fetchNextPage(). So the upshot is that everything should behave just like it always has, except we now run an immediate query to keep posts fresh whenever a posts-list component is re-mounted or the page has re-gained focus.

Ready for another look.

This should address stale data that can otherwise be shown in the desktop app.
@rralian rralian force-pushed the update/post-list-fetch-on-mount branch from cdaaf03 to 1161110 Compare February 5, 2016 21:54
@gwwar
Copy link
Contributor

gwwar commented Feb 5, 2016

🚢

@gwwar gwwar 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 5, 2016
@rralian rralian added this to the Calypso Core: Offline 3 milestone Feb 5, 2016
rralian added a commit that referenced this pull request Feb 5, 2016
Posts: ensure a fetch on mounting for post-list-fetcher
@rralian rralian merged commit 327a8ef into master Feb 5, 2016
@rralian rralian deleted the update/post-list-fetch-on-mount branch February 5, 2016 22:17
rralian added a commit that referenced this pull request Feb 6, 2016
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.
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.

5 participants