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

Auto pagination #260

Merged
merged 25 commits into from Mar 5, 2018

Conversation

@georgiwe
Copy link
Contributor

commented Mar 1, 2018

Description

MLIBZ-2045. Implement auto pagination. Unit tests are pending.

Changes

Delete unused file. Small change for eslint. Small improvements on jsdocs. Change the way defaults are set for Query class, so undefined values don't overwrite null values (which causes runtime errors).

return this.pull(query, options);
})
.then((pullResult) => {
result.pull = pullResult;

This comment has been minimized.

Copy link
@tejasranade

tejasranade Mar 1, 2018

Member

The signature of the method suggests the return parameters as an array (<{push: [], pull: []}>). This will return a number, if I understand it correctly.

This comment has been minimized.

Copy link
@georgiwe

georgiwe Mar 2, 2018

Author Contributor

Yeah, that's from "the old branch", I'll update it.

@@ -60,6 +66,12 @@ export class SyncManager {
if (!isNonemptyString(collection)) {
return Promise.reject(new KinveyError('Invalid or missing collection name'));
}

// TODO: decide on default value of pagination setting
if (options && (options.autoPagination && !options.useDeltaFetch)) {

This comment has been minimized.

Copy link
@tejasranade

tejasranade Mar 1, 2018

Member

Using useDeltaFetch here seems incorrect. Whether to get the entire set of objects or a delta, should be decided based on the useDeltaFetch value and the _QueryCache.

If I have run this query before (regardless of whether I ran it as "auto-paginated" or not) and I have delta sync turned on, then make a delta request for me.

This comment has been minimized.

Copy link
@georgiwe

georgiwe Mar 2, 2018

Author Contributor

Well yeah, but delta set isn't implemented here yet. This will have to be done after delta set is ready. We only have the old dateset here, and this is how it works.

This comment has been minimized.

Copy link
@georgiwe

georgiwe Mar 2, 2018

Author Contributor

Also, what's the decision on the setting - setting, no setting, default value?

This comment has been minimized.

Copy link
@thomasconner

thomasconner Mar 2, 2018

Contributor

Can you explain what you mean "setting, no setting, default value"?

This comment has been minimized.

Copy link
@georgiwe

georgiwe Mar 2, 2018

Author Contributor

There is a discussion in the second spec doc about whether auto pagination should be a setting or not. And if it is a setting, what should be its default value? There is no final decision yet. I've left it as a setting, with a default value of false, as per the original spec doc.

query.limit = totalCount;
const noSortProvided = !sort || isEmpty(sort);
const noSkipLimitProvided = !userQuery.skip && !userQuery.limit;
if (noSortProvided || noSkipLimitProvided) {

This comment has been minimized.

Copy link
@tejasranade

tejasranade Mar 1, 2018

Member

Shouldn't this be &&? The way it reads - if a user provided a sort, but not a skip/limit, we will replace their sort with ours.

This comment has been minimized.

Copy link
@georgiwe

georgiwe Mar 2, 2018

Author Contributor

That's right. If they want a sort, but don't care about the paging, they shouldn't care about the sort either, since they are not getting the entities anymore. It's in the doc.

This comment has been minimized.

Copy link
@thomasconner

thomasconner Mar 2, 2018

Contributor

This doesn't make sense to me. I read the doc but I am still confused here.

This comment has been minimized.

Copy link
@georgiwe

georgiwe Mar 2, 2018

Author Contributor

We can discuss it at standup. It's the second bullet point of the section about sort.

@georgiwe georgiwe removed the DO NOT MERGE label Mar 2, 2018

@@ -99,12 +99,15 @@ export class CacheStore extends NetworkStore {
* Pull items for the data store from the network to your local cache. A promise will be
* returned that will be resolved with the result of the pull or rejected with an error.
*
* IMPORTANT: This method is not intended to be used to make concurrent requests.

This comment has been minimized.

Copy link
@georgiwe

georgiwe Mar 2, 2018

Author Contributor

This is about MLIBZ-2130. You may want to document this better, I think this is what we agreed upon in the comments of the ticket.

@@ -1,153 +0,0 @@
import { KinveyError, NotFoundError } from '../../errors';

This comment has been minimized.

Copy link
@georgiwe

georgiwe Mar 2, 2018

Author Contributor

This file is not used anywhere, it's an outdated file, left from all the rebasing during the redesign. In case anyone is wondering :)

},

[platformName.phoneGap]: {
maxConcurrentPullRequests: mobilePlatformsConcurrentPulls

This comment has been minimized.

Copy link
@georgiwe

georgiwe Mar 2, 2018

Author Contributor

I haven't tested on PhoneGap. I set it to the same as NativeScript, just in case. We can test it out before GA.

@@ -800,7 +801,7 @@ export class Query {

// Remove fields
if (Array.isArray(json.fields) && json.fields.length > 0) {
const fields = Array.concat([], json.fields, PROTECTED_FIELDS);
const fields = [].concat(json.fields, PROTECTED_FIELDS);

This comment has been minimized.

Copy link
@georgiwe

georgiwe Mar 2, 2018

Author Contributor

This will cause a conflict once #258 gets merged. I haven't merged 258 cause of Tejas' request for changes. I keep forgetting to bring this up.

@@ -59,6 +59,7 @@
return networkStore.save(entity);
}))
.then(() => syncStore.pull())
.then(() => syncStore.find().toPromise())

This comment has been minimized.

Copy link
@georgiwe

georgiwe Mar 2, 2018

Author Contributor

@tsvetomir-nedyalkov please have a look at this and make sure it won't change the behaviour of the method. The result type of pull() has changed and it shouldn't be used like it is being used here. Yes, it is a breaking change, but it's been discussed and approved.

@@ -181,6 +182,11 @@
.then(() => syncStore.clear());
}

function cleanAndPopulateCollection(collectionName, entities) {

This comment has been minimized.

Copy link
@georgiwe

georgiwe Mar 2, 2018

Author Contributor

@tsvetomir-nedyalkov I noticed cleanUpCollectionData() and saveEntities() are almost always used together, so I added this helper method.

@thomasconner thomasconner merged commit 2ab15ff into master Mar 5, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@thomasconner thomasconner deleted the auto-pagination-extended branch Mar 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.