fix: allCollections query pagination and pagetype timeout errors #1140
Conversation
|
✔️ Deploy Preview for faststoreui ready! 🔨 Explore the source changes: 6e18a24 🔍 Inspect the deploy log: https://app.netlify.com/sites/faststoreui/deploys/6213a6585a352f00072db77d 😎 Browse the preview: https://deploy-preview-1140--faststoreui.netlify.app |
|
✔️ Deploy Preview for faststore-docs ready! 🔨 Explore the source changes: 6e18a24 🔍 Inspect the deploy log: https://app.netlify.com/sites/faststore-docs/deploys/6213a658947b7a00074aa2d7 😎 Browse the preview: https://deploy-preview-1140--faststore-docs.netlify.app |
icazevedo
force-pushed
the
fix/wrong-pagination-and-pagetype-timeouts
branch
from
e51541e to
91e6304
Compare
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 6e18a24:
|
icazevedo
changed the title
|
|
||
| const loader = (slugs: readonly string[]) => { | ||
| return Promise.all( | ||
| slugs.map((s: string) => |
There was a problem hiding this comment.
why not using p-map in here? this way we don't need this Promise.all chained with a map chained with a limit
There was a problem hiding this comment.
p-limit-ing there would allow more than 20 requests to be executed at the same time, and we want to avoid just that.
icazevedo
force-pushed
the
fix/wrong-pagination-and-pagetype-timeouts
branch
from
2dbe1ce to
8c1f17b
Compare
icazevedo
force-pushed
the
fix/wrong-pagination-and-pagetype-timeouts
branch
from
eb7d596 to
f613efc
Compare
| const pageTypes = await Promise.all( | ||
| slugs.map((s) => commerce.catalog.portal.pagetype(s)) | ||
| const pageTypePromises = await Promise.allSettled( | ||
| slugs.map((slugSegment) => pagetypeLoader.load(slugSegment)) |
There was a problem hiding this comment.
What do you think about adding this pagetypeLoader inside the commerce.catalog.portal.pagetype?
There was a problem hiding this comment.
I don't like it. It deviates from what we're currently doing with the SKU and Simulation loaders and couples logic that I think would be good to keep separate.
| } = ctx | ||
|
|
||
| const result = await commerce.catalog.portal.pagetype(slug) | ||
| const result = await pagetypeLoader.load(slug) |
There was a problem hiding this comment.
Why not put it inside the commerce.catalog.portal
|
This PR fixes lots of the issues faced with missing collections. However, there will be still issues. These issues mainly come from the slugify function that we use. We use sindresorhu's slugify function to match what gatsby uses internally. However, pageType api uses the catalog slugify function. Because of how Gatsby works internally, this ambiguity will break some categories. Gatsby v4, however, fixes these issues. In the future, once we launch Gatsby v4, we can properly fix these other issues. |
What's the purpose of this pull request?
This PR fixes pagination for the allCollections request and also fixes timeouts during pagetype fetching. It also fixes a bug where some products could be ignored during fetching.
How it works?
For fixing pagination, it sends the proper
hasNextPageandcursordata with the result.For fixing timeouts during pagetype fetching, it uses a new DataLoader for pagetypes, which dramatically reduces the total amount of requests being made because of its cache. Also, limits the amount of concurrent pagetype requests being made.
How to test it?
I'll link the base.store PR below
base.storeDeploy PreviewTBA
References
We're limiting to 20 concurrent pagetype requests by recommendation from @tlgimenes. The pagination is compliant with Relay's specification.