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

Another fetchbatched fix #2

Merged
merged 14 commits into from
Feb 14, 2019
Merged

Another fetchbatched fix #2

merged 14 commits into from
Feb 14, 2019

Conversation

tehpsalmist
Copy link
Contributor

This introduces a breaking change to the fetchBatched method, in that the second param will be a full params object (which can included a filter, of course), instead of only a filter object. This allows the developer to customize important params like limit, attributes, and filter params (e.g. not-field123, contains-string, etc).

Another method was also added: fetchBatchedPaginated, which mirrors fetchBatched except that the return value will be wrapped in an Array, and each item in that Array will correspond to a page of data (also an Array), the length of which is defined by the limit param.

For both methods, if the limit param provided is not a valid number for that resource, the corrected limit that will be returned from Zengine is used to complete the rest of the recursive data-gathering, to ensure that each page is equal in length (except the last one) and all records are accurately obtained from the API.

One final note on the implementation: previously the fetchBatched method relied on Promise.all to get the remaining pages, but that is not very API friendly for large datasets, so the recursive pattern was used instead.

@coveralls
Copy link

coveralls commented Feb 12, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling b95c556 on another-fetchbatched-fix into e822b08 on master.

@tehpsalmist
Copy link
Contributor Author

Thinking about releasing this as 2.0.0 because of the breaking change on that method, but maybe that's not how semver works. Anyone with better insight on that?

@AParks
Copy link
Contributor

AParks commented Feb 13, 2019

@tehpsalmist if you include the words "BREAKING CHANGE" in one of your commits, then the release script will know to do a major version bump - https://github.com/conventional-changelog/standard-version#commit-message-convention-at-a-glance

@tehpsalmist
Copy link
Contributor Author

Thanks for the tip! So I guess you agree then that this should be a 2.0.0 release?

@AParks
Copy link
Contributor

AParks commented Feb 13, 2019

@tehpsalmist whats your use case for fetchBatchPaginated?

@tedkulp
Copy link

tedkulp commented Feb 13, 2019

Since custom dev is going to start making heavy use of jobs/batching, the idea is that we can pull the records in a way that's already setup to create all the job's batches without having to recalc and split all the records into usable chunks.

@AParks
Copy link
Contributor

AParks commented Feb 13, 2019

ah makes sense thanks @tedkulp

@tehpsalmist
Copy link
Contributor Author

What will it take to see this merged? I'm all ears.

Copy link
Contributor

@evert0n evert0n left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@tehpsalmist
Copy link
Contributor Author

Thanks y'all. @AParks I'll get some concurrency in those requests eventually, I promise.

@tehpsalmist tehpsalmist merged commit 89b8367 into master Feb 14, 2019
@tehpsalmist tehpsalmist deleted the another-fetchbatched-fix branch February 14, 2019 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants