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

Add initial implementation #3

Merged
merged 31 commits into from
Apr 25, 2016
Merged
Changes from 8 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
9681a81
Add initial implementation
Mar 22, 2016
9d27a83
Refactor module imports
Mar 22, 2016
476f810
Refactor the paginator to cache promises instead of responses
Mar 22, 2016
c857f21
Update dependency list
Mar 22, 2016
e844e6c
Merge master
Apr 11, 2016
cc38b8f
Refactor paginator into seperate modules
Apr 11, 2016
1814d4e
Minor fixes
Apr 11, 2016
e5be062
Update npm scripts
Apr 12, 2016
824c2f5
Update documentation
Apr 12, 2016
99da1be
Remove unused files
Apr 14, 2016
dfbf7cb
Update package.json
Apr 14, 2016
a75dc94
Remove unused whatwg-fetch devDependency
Apr 14, 2016
54ea997
Correct invalid page handling
Apr 14, 2016
d5ee103
Add coverage reporting
Apr 14, 2016
0e0fadd
Tweak docs
Apr 14, 2016
e664deb
Correct test descriptions and typos
Apr 14, 2016
5b32e88
Update comments in API doc to be REPL like
Apr 14, 2016
ea48a9b
Remove Karma
Apr 21, 2016
39dad7e
Remove unused test code
Apr 21, 2016
33c9f5e
Rename variables for clarity
Apr 21, 2016
91bce54
Add default values to the query handlers
Apr 21, 2016
b57de47
Simplify resolvePage to a single purpose
Apr 21, 2016
7b628b2
Add tests for 100% coverage
Apr 21, 2016
9de4f7b
Add line breaks
Apr 21, 2016
632f79d
Correct grammar
Apr 21, 2016
8dc65c7
Update invalid page handling to reject an error
Apr 21, 2016
088ced4
Add missing semicolon
Apr 21, 2016
5b1b7f3
Simplify actions
Apr 22, 2016
ddc43d8
Change absolute modules paths to be relative
Apr 25, 2016
3a22f42
Remove unneeded app-module-path dependency
Apr 25, 2016
670abb9
Remove the browserify paths option
Apr 25, 2016
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 25 additions & 23 deletions API.md
Original file line number Diff line number Diff line change
@@ -50,14 +50,13 @@

#### paginator.page

This is a [getter][mdn-defineproperty] property.

[mdn-defineproperty]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/defineProperty
This is a property is read-only.
Copy link
Contributor

Choose a reason for hiding this comment

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

Grammarize

Copy link
Contributor

Choose a reason for hiding this comment

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

Github hid it. Grammarize this.


```javascript
let paginator = new BasePaginator(request);
// True

paginator.page === 0;
// -> true
```

#### paginator.queryHandler
@@ -192,10 +191,10 @@ promise.then((mergedResponse) => {
```javascript
let error = new PaginatorError('Something went wrong.');

// True
error instanceof PaginatorError;
// True
// -> true
error instanceof Error;
// -> true
```

## Paginators
@@ -226,8 +225,9 @@ new PageNumberPaginator(request[, requestOptions, queryParams]);
```javascript
let queryParams = { page: 2 };
let paginator = new PageNumberPaginator(request, null, queryParams);
// True

paginator.page === 2;
// -> true
```

### LimitOffsetPaginator
@@ -241,8 +241,9 @@ new LimitOffsetPaginator(request[, requestOptions, queryParams]);
```javascript
let queryParams = { limit: 50, offset: 50 };
let paginator = new LimitOffsetPaginator(request, null, queryParams);
// True

paginator.page === 2;
// -> true
```

## Query Handlers
@@ -275,8 +276,8 @@ _Example:_
let options = { pageQueryParam: 'p' };
let queryHandler = new PageNumberQueryHandler(options);
let queryParams = queryHandler.makeParams(42);
// True
queryParams.p === 42;
// -> true
```

[drf-page-number-config]: http://www.django-rest-framework.org/api-guide/pagination/#configuration
@@ -320,10 +321,11 @@ let queryHandler = new LimitOffsetQueryHandler(options);
queryHandler.setParams({ l: 50 });

let queryParams = queryHandler.makeParams(3);
// True

queryParams.l === 50;
// True
queryParams.o === 100;
// -> true
queryParams.o === 100
// -> true;
```

## Utility
@@ -373,7 +375,7 @@ import querystring from 'querystring';
let request = function(options, queryParams) {
let {user} = options;
let query = querystring.stringify(queryParams);
let url = `https://example.com/users/${user/}?${query}`;
let url = `https://example.com/users/${user}?${query}`;

return fetch(url)
.then((response) => response.json());
@@ -399,37 +401,37 @@ import {requests} from 'user-image-service';
/**
* Create a paginator for the endpoint using an existing request function
*/
const userPhotoUrls = new drfp.LimitOffsetPaginator(
requests.fetchPhotoUrls,
const userImageUrls = new drfp.LimitOffsetPaginator(
requests.fetchImageUrls,
{ user: 'abc123' },
{ limit: 20 }
);

/**
* Create a request function that retrieves and merges ten pages of results
*/
const batchFetchPhotoUrls = function(options, queryParams) {
const batchFetchImageUrls = function(options, queryParams) {
const {page} = queryParams;
const startPage = page * 10 - 9;
const endPage = page * 10;
const pageMerger = new drfp.PageMerger(userPhotoUrls);
const pageMerger = new drfp.PageMerger(userImageUrls);

return pageMerger.merge(startPage, endPage);
};

/**
* Create a paginator for the new request function
* Create a paginator for the batch request function
*/
let batchUserPhotoUrls = drfp.paginate(batchFetchPhotoUrls);
const batchUserImageUrls = drfp.paginate(batchFetchImageUrls);

const displayNextImages = function() {
return batchUserPhotoUrls.next()
.then(displayPhotos)
return batchUserImageUrls.next()
.then(displayImages)
.catch(displayError);
};

/**
* Display the first page of 200 images
* Display the first page of 200 Images
*/
showNextImages();
displayNextImages();
```
13 changes: 12 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
@@ -65,6 +65,11 @@ Please view the [API reference][api] for details and more examples.

A [Promises/A+][promise-spec] compliant promise implementation must be available globally.

Consumers using [Browserify](browserify) must have the [Babelify](babelify) transform.

[babelify]: https://github.com/babel/babelify
[browserify]: http://browserify.org/

## Installation

Node.js via [npm](https://www.npmjs.com)
@@ -87,7 +92,7 @@ Install dependencies
$ npm install
```

Run tests and lint
Run the test suite, generate coverage reports, and lint the source

```bash
$ npm test
@@ -105,6 +110,12 @@ Run unit tests
$ npm run unit
```

Run the test suite, and generate coverage reports

```bash
$ npm run cover
```

Publish coverage report

```bash
33 changes: 0 additions & 33 deletions karma.unit.conf.js

This file was deleted.

36 changes: 14 additions & 22 deletions package.json
Original file line number Diff line number Diff line change
@@ -7,10 +7,11 @@
"license": "MIT",
"main": "src/index.js",
"scripts": {
"cover": "rm -rf coverage && babel-node node_modules/.bin/isparta cover --root src/ --report text --report lcov --report html node_modules/.bin/_mocha -- src/test-setup.js $(find src -name '*-spec.js')",
"coveralls": "cat coverage/lcov.info | coveralls",
"jshint": "jshint src",
"test": "karma start karma.unit.conf.js && jshint src",
"unit": "karma start karma.unit.conf.js"
"test": "npm run cover && npm run jshint",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want linting to break tests. Snitch does that for us, and because linting errors are often stylelistic, they shouldn't affect tests.

Do you agree?

Copy link
Contributor Author

@morrisallison morrisallison Apr 22, 2016

Choose a reason for hiding this comment

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

I remember @thomasw saying we ought to, but didn't find a comment after some searching. It is ran in SBUI though.

"unit": "babel-node node_modules/.bin/_mocha --require src/test-setup.js --recursive src"
},
"keywords": [
"django",
@@ -28,46 +29,37 @@
"url": "https://github.com/yola/drf-paginator/issues"
},
"dependencies": {
"babel-preset-es2015": "~6.6.0",
"lodash.assign": "~4.0.6",
"lodash.clonedeep": "~4.3.2",
"lodash.omit": "~4.1.0",
"lodash.reduce": "~4.3.0"
},
"devDependencies": {
"babel-preset-es2015": "~6.6.0",
"babelify": "~7.2.0",
"app-module-path": "~1.0.6",
"babel-cli": "~6.7.5",
"bluebird": "~3.3.4",
"browserify": "~13.0.0",
"chai": "~3.5.0",
"chai-as-promised": "~5.3.0",
"coveralls": "~2.11.9",
"isparta": "~4.0.0",
"jsgreat": "~0.1.8",
"jshint": "~2.9.1",
"karma": "~0.13.22",
"karma-browserify": "~5.0.3",
"karma-coverage": "~0.5.5",
"karma-mocha": "~0.2.2",
"karma-phantomjs-launcher": "~1.0.0",
"karma-spec-reporter": "0.0.24",
"mocha": "~2.4.5",
"phantomjs-prebuilt": "~2.1.6",
"sinon": "~1.17.3",
"sinon-chai": "~2.8.0",
"watchify": "~3.7.0"
"sinon-chai": "~2.8.0"
},
"babel": {
"presets": [
"es2015"
]
},
"browserify": {
"paths": [
"."
],
"transform": [
[
"babelify",
{
"presets": [
"es2015"
]
}
]
"babelify"
]
}
}
46 changes: 46 additions & 0 deletions src/actions-spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import chai from 'chai';

import actions from 'src/actions';


const {expect} = chai;

describe('drf-paginator', function() {
const response = {
count: 0,
results: []
};

describe('actions', function() {
it('should export inferResponseLimit', function() {
expect(actions.inferResponseLimit).to.exist;
});

it('should export parseResponse', function() {
expect(actions.parseResponse).to.exist;
});

describe('inferResponseLimit', function() {
it('returns NULL when it can\'t determine the limit', function() {
const result = actions.inferResponseLimit(response);

expect(result).to.be.null;
});
});

describe('parseResponse', function() {
it('parses responses', function() {
const result = actions.parseResponse(response);

const expectedResult = {
count: 0,
limit: null,
pageCount: 1,
results: response.results
};

expect(result).to.eql(expectedResult);
});
});
});
});
12 changes: 6 additions & 6 deletions src/actions.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
export const inferResponseLimit = function(data) {
const count = data.count;
const resultCount = data.results.length;
export const inferResponseLimit = function(response) {
const count = response.count;
const resultCount = response.results.length;

if (count > resultCount) {
Copy link

Choose a reason for hiding this comment

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

what's a scenario where this is true, and why would that arise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When a query has multiple pages, that expression will always be true for the first page response.
The times when it's not true, is on the last page, or when there are no results.

return resultCount;
@@ -9,9 +9,9 @@ export const inferResponseLimit = function(data) {
return null;
};

export const parseResponse = function(data) {
const {count, results} = data;
const limit = inferResponseLimit(data);
export const parseResponse = function(response) {
Copy link

Choose a reason for hiding this comment

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

I'm curious why this is called parseResponse when it seems to just be adding page count and limit attributes to the response?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I called it parse since it's only pulling out the properties of the response it cares about, and creating a new object.

However, I do see where you're coming from. The function is really only calculating the pageCount. I'll refactor it to do only that.

const {count, results} = response;
const limit = inferResponseLimit(response);
let pageCount = 1;

if (count && limit) {
22 changes: 0 additions & 22 deletions src/fixtures/limit-offset-response.json

This file was deleted.

32 changes: 1 addition & 31 deletions src/helpers/request-spy.js
Original file line number Diff line number Diff line change
@@ -1,48 +1,18 @@
import assign from 'lodash.assign';
import sinon from 'sinon';

import limitOffsetResponse from 'src/fixtures/limit-offset-response.json';
import pageNumberResponse from 'src/fixtures/page-number-response.json';


export const request = function(options, queryParams) {
const uniqueValue = Math.random();
let fixture;

if (queryParams.hasOwnProperty('page')) {
fixture = pageNumberResponse;
} else {
fixture = limitOffsetResponse;
}

const response = assign({}, fixture, { uniqueValue });
const response = assign({}, pageNumberResponse, { uniqueValue });

return Promise.resolve(response);
};

export const spy = sinon.spy(request);

export const getArgs = function() {
const args = spy.args;
return args[args.length - 1];
};

export const getCallCount = function() {
return spy.callCount;
};

export const getOptions = function() {
return getArgs()[0];
};

export const getQueryParams = function() {
return getArgs()[1];
};

export const getOffset = function() {
return getQueryParams().offset;
};

export const getPage = function() {
return getQueryParams().page;
};
14 changes: 14 additions & 0 deletions src/paginators/base-paginator-spec.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import chai from 'chai';
import chaiAsPromised from 'chai-as-promised';
import sinon from 'sinon';
import sinonChai from 'sinon-chai';

import {BasePaginator, errors} from 'src/paginators/base-paginator';
@@ -303,6 +304,11 @@ describe('drf-paginator', function() {
describe('fetchPageCount', function() {
beforeEach(function() {
makePaginator();
sinon.spy(paginator, 'fetchPage');
});

afterEach(function() {
paginator.fetchPage.restore();
});

it('returns a promise that resolves', function() {
@@ -316,6 +322,14 @@ describe('drf-paginator', function() {

return expect(pageCount).to.eventually.equal(10);
});

it('caches the page count', function() {
const fetchPageCallCount = paginator.fetchPageCount()
.then(() => paginator.fetchPageCount())
.then(() => paginator.fetchPage.callCount);

return expect(fetchPageCallCount).to.eventually.equal(1);
});
});

describe('setQueryHandler', function() {
2 changes: 2 additions & 0 deletions src/paginators/base-paginator.js
Original file line number Diff line number Diff line change
@@ -51,13 +51,15 @@ export class BasePaginator {
return this._getRequest(page)
.then(cloneDeep);
}

if (!this._isValidPage(page)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

linebreak

return this._rejectPage(page);
}

const handleResponse = (response) => {
return this._handleResponse(response, page);
};

const request = this._sendRequest(page)
Copy link
Contributor

Choose a reason for hiding this comment

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

linebreak

.then(handleResponse);

51 changes: 51 additions & 0 deletions src/query-handlers/limit-offset-query-handler-spec.js
Original file line number Diff line number Diff line change
@@ -101,6 +101,26 @@ describe('drf-paginator', function() {

expect(page).to.equal(42);
});

it('throws an error if no limit is given',
function() {
const check = function() {
handler.resolvePage({
offset: 20500
});
};

expect(check).to.throw(PaginatorError);
});

it('resolves the first page without a limit being set',
function() {
const page = handler.resolvePage({
offset: 0
});

expect(page).to.equal(1);
});
});

describe('setParams', function() {
@@ -111,6 +131,37 @@ describe('drf-paginator', function() {
});
});

describe('onResponse', function() {
it('sets the limit query parameter if missing', function() {
const response = {
count: 10,
results: Array(2)
};
const page = 1;

handler.onResponse(response, page);

const queryParams = handler.makeParams(1);

expect(queryParams.limit).to.equal(2);
});

it('doesn\'t set the limit query parameter if the page isn\'t one',
function() {
const response = {
count: 10,
results: Array(2)
};
const page = 2;

handler.onResponse(response, page);

const queryParams = handler.makeParams(1);

expect(queryParams.limit).to.be.null;
});
});

describe('setOptions', function() {
it('is a fluent method', function() {
var result = handler.setOptions({ hello: 'world' });
34 changes: 13 additions & 21 deletions src/query-handlers/limit-offset-query-handler.js
Original file line number Diff line number Diff line change
@@ -17,6 +17,8 @@ const defaultOptions = {

export class LimitOffsetQueryHandler {
constructor(options) {
this._excessParams = null;
this._limit = null;
this.setOptions(options);
}

@@ -31,13 +33,17 @@ export class LimitOffsetQueryHandler {
}

resolvePage(queryParams) {
const result = this._parse(queryParams);
const {offset, limit} = this._parse(queryParams);

if (!this._limit) {
this._limit = result.limit;
if (offset === 0) {
return 1;
}

return this._calculatePage(result.offset);
if (!limit) {
throw new PaginatorError(errors.calculatePageNoLimit);
}

return (offset + limit) / limit;
}

setParams(queryParams) {
@@ -48,10 +54,9 @@ export class LimitOffsetQueryHandler {
return this;
}

onResponse(response) {
const result = actions.parseResponse(response);

if (!this._limit) {
onResponse(response, page) {
if (page === 1 && !this._limit) {
const result = actions.parseResponse(response);
this._limit = result.limit;
}
}
@@ -104,19 +109,6 @@ export class LimitOffsetQueryHandler {

return limit * page - limit;
}

_calculatePage(offset) {
const limit = this._limit;

if (offset === 0) {
return 1;
}
if (!limit) {
throw new PaginatorError(errors.calculatePageNoLimit);
}

return (offset + limit) / limit;
}
}

export default LimitOffsetQueryHandler;
1 change: 1 addition & 0 deletions src/query-handlers/page-number-query-handler.js
Original file line number Diff line number Diff line change
@@ -8,6 +8,7 @@ const defaultOptions = {

export class PageNumberQueryHandler {
constructor(options) {
this._excessParams = null;
this.setOptions(options);
}

4 changes: 3 additions & 1 deletion src/test-setup.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import appModulePath from 'app-module-path';
import bluebird from 'bluebird';


window.Promise = bluebird;
appModulePath.addPath(__dirname + '/..');
global.Promise = bluebird;