implement #138 since option for collection.getTotalRecords #142
Conversation
| * @param {Object} [options={}] The options object. | ||
| * @param {Object} [options.headers] The headers object option. | ||
| * @return {Promise<Number, Error>} | ||
| */ | ||
| getTotalRecords(options={}) { | ||
| getTotalRecords(since, options={}) { |
There was a problem hiding this comment.
I think we should follow what was done in listRecords() : make since an option. Plus, it wouldn't break the API!
| method: "HEAD", | ||
| path: "/buckets/blog/collections/posts/records?since=ETAG", | ||
| }, {raw: true}); | ||
| }); |
| return this.client.execute({ | ||
| method: "HEAD", | ||
| path: endpoint("record", this.bucket.name, this.name), | ||
| path: path, |
There was a problem hiding this comment.
I insist on this, pretty please:
return this.client.execute({
method: "HEAD",
path,
headers,
}, {raw: true})There was a problem hiding this comment.
I've used path directly. Is there something else to do?
There was a problem hiding this comment.
That's basically the difference between:
return this.client.execute({
method: "HEAD",
path: path,
headers,
}, {raw: true})and
return this.client.execute({
method: "HEAD",
path,
headers,
}, {raw: true})| * @return {Promise<Number, Error>} | ||
| */ | ||
| getTotalRecords(options={}) { | ||
| const { headers } = this._collOptions(options); | ||
| let path = endpoint("record", this.bucket.name, this.name); | ||
| if(options.since){ | ||
| path += "?since="+options.since; |
There was a problem hiding this comment.
It may be nice to reuse the utils.qsify function here.
Also nit: space around operand
| * @return {Promise<Number, Error>} | ||
| */ | ||
| getTotalRecords(options={}) { | ||
| const { headers } = this._collOptions(options); | ||
| let path = endpoint("record", this.bucket.name, this.name); | ||
| if(options.since){ |
There was a problem hiding this comment.
nit: space after if and before {
| @@ -79,13 +79,18 @@ export default class Collection { | |||
| * | |||
| * @param {Object} [options={}] The options object. | |||
| * @param {Object} [options.headers] The headers object option. | |||
| * @param {String} [options.since] The since ETag option. | |||
| * @return {Promise<Number, Error>} | |||
| */ | |||
| getTotalRecords(options={}) { | |||
| const { headers } = this._collOptions(options); | |||
There was a problem hiding this comment.
So that means since is passed to _collOptions and merged into the instance options. I don't think that it's a problem per se, but explicitely removing it may be nicer:
getTotalRecords(options={}) {
const { since, ...otherOptions } = options;
const { headers } = this._collOptions(otherOptions);
let path = endpoint("record", this.bucket.name, this.name);
if (since) {
path += "?since="+options.since;
}There was a problem hiding this comment.
actually, the const { since, ...otherOptions } = options instruction breaks the firefox build:
https://travis-ci.org/Kinto/kinto-http.js/jobs/163077307#L959
Maybe this issue: browserify/module-deps#93
There was a problem hiding this comment.
Sigh. Oh well.
Edit: just to be clear, that's because this is es7 and neither firefox and babel without specific transforms support this syntax.
getTotalRecords(options) has been released already, so this is a breaking API change but not many have jumped on it I hope...