Add method chaining API for query pagination

[akiroz]

No description provided.

[ben181231]

@akiroz I prefer keeping the function name as limit(num), offset(num) and offset(num).

[akiroz]

@ben181231 but the names are already used by the fields, if I override them it will break backwards compatibility....

[ben181231]

Okay. I see.

[ben181231]

@akiroz Please attach the related documentation update PR to this PR.

[rickmak]

I think we may not want to add this method chaining to Query. Consider what does this mean:

q.limitResults(10).skipResults(2).skipResults(5)

The end result is skipping 2, 5 or 7 result? I think this API create ambiguity.

The current query chaining interface only exist for modifying the internal mutable property. i.e. _predicate and _include. The chaining represent an addition in current API design too. And the addition is often commutative too. i.e. q.equalTo('title', 'First note').notEqualTo('category', 'dangerous') and q.notEqualTo('category', 'dangerous').equalTo('title', 'First note') are equal. This PR will break both expectation for end-user.

So far no setter method(chaining or not) on property that are immutable.

In addition, one way to do a things is better than many ways for doing things. It reduces the pain on support.

[chpapa]

I think this pull request is incomplete, but I am open to the discussion of better support of chain result.

I think the ambigiuty Rick mentioned can be solved by if some parameter such as limit or skip was called twice error will be throw. It is not something very new.

But I also agree that if limit, skip, offset are chainable, people is reasonable to expect other query filter like equalTo are chainable too.

Regarding 1 way to do things is better, I think it depends on how common this way of doing thing is in JavaScript. Which my impression is pretty common, so that's why I am open to that. But we probably need more planning on the API (how to keep backward compatiability or not?) and it should works amongst to whole set of query operator before merge into master

[louischan-oursky]

Maybe it is a little bit off topic but I wrote myself a paginator to handle pagination with skygear.Query. Here is the code

const generatePaginator = ({doQuery}) => ({limit}) => (queryProducer) => {
  const query = queryProducer();
  let offset = 0;
  let isFetching = false;
  let isEnded = false;
  let allRecords = [];
  const next = () => {
    if (isFetching) {
      return Promise.resolve();
    }
    if (isEnded) {
      return Promise.resolve([]);
    }
    isFetching = true;
    return doQuery(query, limit, offset)
      .then(_records => {
        const records = Array.prototype.slice.call(_records);
        isFetching = false;
        offset += records.length;
        if (records.length < limit) {
          isEnded = true;
        }
        if (records.length > 0) {
          allRecords = allRecords.concat(records);
        }
        return records;
      })
      .catch(e => {
        isFetching = false;
        return Promise.reject(e);
      });
  };
  return {
    next,
    get isEnded() {
      return isEnded;
    },
    get isFetching() {
      return isFetching;
    },
    get allRecords() {
      return allRecords;
    },
  };
};

export default generatePaginator;

It is generic so we need some glue code to adapt the skygear.Query interface

const skygearPaginator = generatePaginator({
  doQuery(query, limit, offset) {
    query.limit = limit;
    query.offset = offset;
    return skygear.publicDB.query(query);
  },
});

const paginatorWith50ItemsPerPage = skygearPaginator({limit: 50});

In some react component

componentDidMount() {
  this.paginator = paginatorWith50ItemsPerPage(() => {
    const q = new skygear.Query(skygear.Record.extend('product'));
    return q;
  });
  this.fetch();
}

fetch() {
  this.paginator.next().then(records => this.setState({ records }));
}

onEndReach() {
  // some callback fired by some ListView-like component
  this.fetch();
}

So you don't have to manage offset by yourself.

[akiroz]

@rickmak
what's the difference between:
q.limitResults(10).skipResults(2).skipResults(5)
and:
q.limitResults(10); q.skipResults(2); q.skipResults(5) ?
They are both just as ambiguous and does exactly the same thing.
Both calling styles mutate the record object so nothing is immutable.

I don't understand the problem....
Maybe I'm mistaken on how JS works?

[cheungpat]

@akiroz There are no limitResults and skipResults in master at the moment. You need to propose something that is not/less ambiguous.

[akiroz]

@cheungpat
Hmm... I don't really see why extending the API is ambiguous.
As the user, I think the current API is confusing because some calls are chain-able and some are not.
We should recommend new users to use this chain-able API and output a warning when they try to set the internal fields directly IMO.

Ultimately, I want all calls to be chain-able when I construct my record objects because otherwise people will start writing their own factory functions for building records.

what do you think?

[cheungpat]

@akiroz

Hmm... I don't really see why extending the API is ambiguous.

You are extending the interface, @rickmak mentions that the new interface is ambiguous. You are trying to address the concern by comparing the interface you extended with a hypothetical interface that is based on an interface you extended.

The difference is irrelevant. You still have to address the ambiguity introduced by your extension.

I do not have a vested interest in how the interface should work, but skipResults and limitResults are named in a way that invites people to misinterpret how the interface would work. The ambiguity is stated in @rickmak’s comment.

[rickmak]

I am closing this if no more update.