feat(homepage-posts): infinite scroll

[davisshaver]

All Submissions:

• Have you followed the Newspack Contributing guideline?
• Does your code follow the WordPress' coding standards and VIP Go coding standards?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Closes #748.

How to test the changes in this Pull Request:

1. Go to a block with 'load more' button enabled
2. Enable Infinite Scroll on the block

3. See block on frontend and observe infinite scroll
4. Also try placing multiple instances of the Homepage Posts blocks with the same query attributes + infinite scroll enabled side by side on the same page (for example, in columns like this):

5. On the front-end, confirm that all instances of the block load more posts with infinite scroll, but without showing duplicate posts across all instances. Note that the order in which the block instances load more posts is uncontrolled and just depends on the order in which they fire their fetch requests.

See an example in production here: https://lebtown.com/

Scroll to the bottom of the page and see infinite scroll work.

Other information:

• Have you added an explanation of what your changes do and why you'd like us to include them?

• Have you written new tests for your changes, as applicable?

There is no e2e test framework setup in newspack-blocks. If one was added, I would add tests for this feature.

• Have you successfully ran tests with your changes locally?

n/a

[dkoo]

Hi @davisshaver, thank you for submitting this pull request! In order to consider this for review, we need you to provide some details in the description:

• Why is this feature needed for the Homepage Posts block? Can you provide some examples of use cases where it would be useful (screenshots, links, etc.)?
• Testing instructions: How should the code reviewer(s) functionally test the feature? What behavior and visual feedback should we expect while testing?

The PR description template is a good start and we will only consider PRs that contain all the requested info in the template. For new features like this one, providing more detail about why the feature is needed and why in this particular context would also help speed up the review process.

[davisshaver]

Hi @dkoo I'm not a full-time contributor to work on Newspack so I'll do my best to help here. Your and the other maintainers call whether it's worth merging, but Infinite Scroll has already been requested by several publishers and I think what it does is self-explanatory. #748

[davisshaver]

@dkoo What is the next step to having this reviewed?

[dkoo]

@davisshaver thanks for updating the PR description and providing a live example! This is in our queue for review now.

[dkoo]

@davisshaver thanks again for your patience on this. I've reviewed and tested the code, and the implementation looks solid. However, I did encounter some chaos in the scenario where you can place more than one Homepage Posts block with infinite scroll enabled in side-by-side columns on the same page. In this scenario, the infinite scroll for each would get triggered at around the same time, triggering a race condition between the two and resulting in many duplicate posts being loaded in each instance of the block.

With some quick testing I found that replacing isInfiniteScrolling in view.js with a globally-scoped variable like window.isInfiniteScrolling at least fixes the issue of loading duplicate posts by preventing both blocks from firing load requests at the same time. However, it does create some unpredictability around which block will end up loading which posts, and will also prevent the infinite scroll from happening automatically in some cases where they would otherwise conflict.

Before we proceed with the quick fix I wanted to see if you had any ideas for handling this more gracefully, since Homepage Posts blocks are meant to be able to exist in multiple instances on the same page?

[davisshaver]

Hi @dkoo, thanks very much for taking a look. I agree, that is a bit of a chaotic situation. You can see an example of it here: https://lebtown.com/listings/places/north-cornwall-township/

On the example site, the "load more" functionality appears to start at the same time on both columns. The blocks in this case are configured to be mutually exclusive (using include & exclude tag filters) so we don't have the race condition/duplicate issue you mentioned.

Your solution would address the duplicates race issue, which I hadn't considered at all before. That's very important and I think it's worth testing. I would be curious to see how the UX feels once we add the global check.

In terms of other options, there may be some way for us to use a queue so that we can have the second (or third, fourth, etc) block go into loading/spinner state, but wait for any earlier homepage blocks to finish updating before we actually begin fetching additional posts, and that would allow us to have the "load more" functionality still appear to start at the same time while also solving the duplicates issue. But honestly that solution might be overkill.

[dkoo]

@davisshaver ef7833a includes the quick fix I mentioned above to move isInfiniteScrolling into the global scope. I do like your suggestion of creating a sort of queue, so b9fa826 implements a very basic queue: before running a fetch request, if another one is already in progress, we queue the the request in a global array. Then when any fetch request completes successfully or in error, we run the next queued request, if any exists. This doesn't solve the problem of the order of fetches, but it does allow the fetches to run without race conditions and not conflict with each other, while automatically triggering the next queued fetch. Feel free to pull down these changes and see how they feel on your end.

Tagging @Automattic/newspack-product for another review since I contributed code to this PR.

[davisshaver]

Hi @dkoo, nice work on the race condition fix! I validated locally and moved this to prod just so we could see how it performs. https://lebtown.com/listings/places/north-cornwall-township/

I wish we could have the de-duping work as expected and also avoid staggering the article fetching, but given the tradeoff between getting the de-duping to work as expected and the load time, I think it's the right decision to prioritize de-duping. Thank you for identifying this issue.

The only thing I noticed in code review: Previously, we had checks with isFetching and isInfiniteScrolling which would prevent the same homepage block instance from starting multiple load requests at once. I wonder if we should bring these variables back to go along with the globally-scoped variable and queue. That way we could make sure each homepage block instance can only add at most one callback to the queue.

[dkoo]

The only thing I noticed in code review: Previously, we had checks with isFetching and isInfiniteScrolling which would prevent the same homepage block instance from starting multiple load requests at once. I wonder if we should bring these variables back to go along with the globally-scoped variable and queue. That way we could make sure each homepage block instance can only add at most one callback to the queue.

@davisshaver Good call—if you have the bandwidth to work on a commit, please go ahead! Otherwise I can probably take another look later today. Thanks for the thoughtful feedback.

[davisshaver]

@dkoo Just took a shot at this. The updated code is running here: https://lebtown.com/listings/places/north-cornwall-township/

[dkoo]

Looking good to me—thanks, @davisshaver! 🙇

[davisshaver]

Thanks @dkoo! Please let me know if there's any feedback from other maintainers. I will let you know if I experience any issues with this in prod.

[matticbot]

🎉 This PR is included in version 4.2.0-alpha.4 🎉

The release is available on:

• npm package (@alpha dist-tag)
• GitHub release

Your semantic-release bot 📦🚀