Skip to content

bug: fix pagination double fetch#1381

Merged
devpatil7 merged 15 commits intodevelopfrom
bug/double-loaded-category-images
Jul 24, 2019
Merged

bug: fix pagination double fetch#1381
devpatil7 merged 15 commits intodevelopfrom
bug/double-loaded-category-images

Conversation

@sirugh
Copy link
Contributor

@sirugh sirugh commented Jun 24, 2019

Description

Pagination had some bugs that caused a double fetch. Specifically the usePagination hook initialized with a current page of 0, which is just wrong -- it should default to 1, if anything. So I updated the hook to accept an initialPage param which if omitted will default to 1. Now, when we first load, we check the page query param and use it if defined.

I ran into another issue, which is that the apollo client will actually just throw the GQL errors, and we never caught them. So, for example, if you request page=5 and there are only 4 pages, ApolloClient throws an error and we just break. To fix this I added a try-catch and then some retry logic to the category page.

Additional "bonus" fixes:

  • Also fixed default page size in gallery items that was causing a visual discrepancy between the "loading" state and the final loaded category page (12 items vs 6 default).
  • Also removed some unnecessary cleanup from pagination component and moved it to the category page.

Related Issue

Closes #1297.

Verification Steps

  1. Load your local and click on a category. It should only fetch the 6 images for the category once.
  2. Click on page 2. It should still only fetch the page 2 images only once.
  3. CTRL+R to refresh the page. It should still only fetch the images once.
  4. In the url, make the query param page=65 and press enter. It should update to page=1 and it should only fetch one set of images.

Screenshots / Screen Captures (if appropriate)

Proposed Labels for Change Type/Package

  • major (e.g x.0.0 - a breaking change)
  • minor (e.g 0.x.0 - a backwards compatible addition)
  • patch (e.g 0.0.x - a bug fix)

usePagination now defaults to 1 instead of 0 so this is technically a breaking change.

Checklist:

  • I have updated the documentation accordingly, if necessary.
  • I have added tests to cover my changes, if necessary.

@vercel
Copy link

vercel bot commented Jun 24, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://venia-git-bug-double-loaded-category-images.magento-research1.now.sh

@sirugh sirugh added the version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump. label Jun 24, 2019
Math.floor(
~~getQueryParameterValue({
location,
queryParameter: 'page'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One thing that stands out to me in all of this is that I don't know who the page parameter "belongs" to anymore. We need to read it to get the initialPage value for the usePagination hook we initialize here in category.js, but the only place in which we update the query parameter is within pagination.js, a child component.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a really good observation, related to the question in #1305. I think we need to encapsulate the query parameter handling entirely inside usePagination; it should use useSearchParam under the hood.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made these changes. I'll wait for feedback before fixing the tests.

@sirugh sirugh changed the title bug: fix pagination double fetch!= bug: fix pagination double fetch Jun 24, 2019
@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Jun 24, 2019

Messages
📖 We are currently working on automating the PR metadata checks. Until that time, you may see failures related to labels/description/linked issues/etc even if you have fixed the problem. Failures will persist until the next push (assuming they are fixed).

Generated by 🚫 dangerJS against d45336e

Copy link
Contributor

@zetlen zetlen left a comment

Choose a reason for hiding this comment

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

Excellent change, good debugging, a couple small things. That page URL parameter definitely needs to be abstracted into the hook itself.

Math.floor(
~~getQueryParameterValue({
location,
queryParameter: 'page'
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a really good observation, related to the question in #1305. I think we need to encapsulate the query parameter handling entirely inside usePagination; it should use useSearchParam under the hood.

zetlen
zetlen previously approved these changes Jul 15, 2019
@devpatil7
Copy link
Contributor

@sirugh QA pass, good to merge if review is approved.

@sirugh sirugh force-pushed the bug/double-loaded-category-images branch from a7bea0c to ffddff4 Compare July 19, 2019 16:20
…agination hook to only prefix with _ if namespace is provided
initialPage,
initialTotalPages = 1
} = {}) => {
const searchParam = namespace ? `${namespace}_${parameter}` : parameter;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This only includes an _ if namespace is provided. It was incorrectly using a param of _page.

@jcalcaben jcalcaben mentioned this pull request Jul 24, 2019
2 tasks
@devpatil7 devpatil7 merged commit 53852e3 into develop Jul 24, 2019
@supernova-at supernova-at deleted the bug/double-loaded-category-images branch July 25, 2019 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pkg:peregrine pkg:venia-concept version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug]: Duplicate Image request calls on Category page

7 participants