-
Notifications
You must be signed in to change notification settings - Fork 174
Allow fetching all items with multiple calls instead of a very large one #199
Conversation
Especially usefull on servers limiting the duration of requests (for example Heroku, which limits to 30sec each request). Passing the loop parameter will perform multiple calls until all items are downloaded.
a9bc0d7 to
9c27d2f
Compare
|
Hi @tonoli, thanks for reviewing. Could you tell what's missing in this issue before it could be merged ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to work on this! It can be a pretty good feature for a future release. I've added my review, don't hesitate to comment if you have questions
README.md
Outdated
| api: { qs: { _sort: 'drop_date:DESC' } }, | ||
| loop: 100, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loop is an implementation detail of how we connect to the API. So I'd recommend moving the loop option inside of the api object
README.md
Outdated
|
|
||
| #### Multiple requests instead of a single large one | ||
|
|
||
| If you have many items in a collection and a single API call is a bit heavy for your server, you can split it into multiple requests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| If you have many items in a collection and a single API call is a bit heavy for your server, you can split it into multiple requests. | |
| When you have many items in a collection, a single API call can be a bit heavy for your server. In that case, you can split your data fetching into multiple requests. |
src/fetch.js
Outdated
| const { apiURL, queryLimit, jwtToken, reporter } = ctx; | ||
|
|
||
| const { endpoint, api } = entityDefinition; | ||
| const { endpoint, api, loop } = entityDefinition; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, I recommended getting the loop from the api object
src/fetch.js
Outdated
| ); | ||
|
|
||
| try { | ||
| if (typeof loop === 'number') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also check if it's a integer (no decimals) and >= 1
src/fetch.js
Outdated
| reporter, | ||
| queryLimit, | ||
| requestOptions, | ||
| loop, | ||
| resultsAggreg = [], | ||
| _start = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you replace these arguments with an object? This way it will be easier to read when calling that function
src/fetch.js
Outdated
| reporter, | ||
| queryLimit, | ||
| requestOptions, | ||
| loop, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can make the default value 1
src/fetch.js
Outdated
| const newResults = resultsAggreg.concat(newlyFetched); | ||
| if (newlyFetched.length < loop || newResults.length >= queryLimit) { | ||
| reporter.info(`Loop fetching ended with ${newResults.length} items - ${requestOptions.url}`); | ||
| //ended |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this comment more explicit
src/fetch.js
Outdated
| //ended | ||
| return newResults; | ||
| } | ||
| return fetchLoop(reporter, queryLimit, requestOptions, loop, newResults, _start + loop); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment to make this recursion easier to read
src/index.js
Outdated
| if (typeof type === 'object') { | ||
| return { | ||
| endpoint: type.endpoint || (single ? type.name : pluralize(type.name)), | ||
| loop: type.loop, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be needed if we move it to the API object
|
When can this feature be released? |
|
@remidej Changes made according to suggestions, thanks for your review. The only issue i didn't address is the part y ou ask for an explicit comment about the recursion, if you have an idea that would be nice ? |
|
Hey @remidej, is there anything we can do to so this can be merged? |
|
I would really like to have this. Currently I'm trying to load 90k models, but whatever I try, this this plugin wants to fetch them all at once. I've even tried to split request by adding multiple types in a loop with the same name with |
|
We're also needing this to be able to deploy on heroku. Is the only change blocking a missing comment ? |
|
I am closing the PR as a new version has been released, feel free to reopen it. |
|
Since v2 is only compatible for Strapi 4, this fix would still be highly appreciated for us using Strapi 3 (MongoDB say hello 👋) |
|
Would it be possible for this to be merged in as |
|
I've been running this code for a while now using I just realized that on incremental builds on Gatsby Cloud, the loop isn't running. My assumption here is that Gatsby Cloud incremental builds (which detect a Strapi webhook) run their installed version of the source plugin, not my patched one. So still I would really really really love to see this feature released as a version If this PR needs any work still, I would be super happy to work on it. |
|
@laurenskling have tried to the new version with the Strapi V4? It does support Gatsby incremental build |
|
@soupette yes I know the v4 version is updated, but i'm running 10+ codebases on Strapi 3, and for all of them I need to: switch from mongo to sql, change all database fields to snake_case, update all my code, update all my plugins code, update my frontend code to use the new GraphQL output. It's not something I can do overnight, it will take me months, many months, before I'm ready to fully switch to v4. |
|
Closing this as it's targeting Strapi v3, which reaches end of life in a month, so we won't maintain the v3 version of this source plugin either |
Fix #104
Especially usefull on servers limiting the duration of requests (for example Heroku, which limits to 30sec each request).
Passing the loop parameter will perform multiple calls until all items are downloaded.
Readme.md updated with an example.