Add initial implementation #3

morrisallison · 2016-03-22T18:15:40Z

Parents: #1 #2

Additional properties and methods were added, but I only consider clone() to be supplementary. I'm working on writing integration tests for a tiny DRF service.

🔔 ping @yola/frontend-engineering

morrisallison · 2016-03-22T18:23:58Z

One point of concern is when calling next(), prev(), first(), last(), or current() repeatedly without waiting for requests to finish. I was going to add a request queue, but I figured I grab some opinions first.

LionOps · 2016-03-28T18:22:52Z

.jshintignore

@@ -0,0 +1,33 @@
+# Logs


Is there a point to this file? Snitch doesn't need it, and all of our other libs don't have one either. If Snitch did need it, it'd make sense, but I think it can go. If you do see a use case, share.

This file is the same as the gitignore, why not symlink it?

My bad, I was using it for local changes not in this PR yet. It isn't needed (yet?)

LionOps · 2016-03-30T00:19:55Z

One point of concern is when calling next(), prev(), first(), last(), or current() repeatedly without waiting for requests to finish. I was going to add a request queue, but I figured I grab some opinions first.

How would the request queue work? I think that calling them repeatedly should return a promise for the next page. There is a risk that next is called more times than actual pages, but I think we can have documented behavior around that. Calling next() on a page that doesn't exist should resolve to the same error regardless if it's a known limit or not. Knowing the page count should merely prevent the request actually being made.

beck · 2016-03-30T20:52:30Z

README.md

+
+
+const limit = 50;
+const style = Paginator.styles.limitOffset;


Why does the paginator need a configured "style"? Shouldn't the behavior be determined by given options?

The style option determines what queryParams are sent to the request. Requests aren't supposed to interpret the query params, but only serialize them. This way the paginator is configured to match the request rather than the other way around.

beck · 2016-04-01T16:25:40Z

I'm a little surprised a class is used instead of a more functional approach. Notice that there is a proposal for Async Iterator for which FB has already written a polyfill? Not saying this is the needed approach, but it would feel more contemporary.

beck · 2016-04-01T16:36:46Z

README.md

+  - Indicates which pagination style to use.
+  - Currently supports:
+    - [`PageNumberPagination`](http://www.django-rest-framework.org/api-guide/pagination/#pagenumberpagination)
+    - [`LimitOffsetPagination`](http://www.django-rest-framework.org/api-guide/pagination/#limitoffsetpagination)


Testing our endpoints, do we support LimitOffsetPagination anywhere? Supporting alternate pagination styles seems like a feature that could come after the initial release.

No, it's not needed now. However, when writing the paginator, I wanted to ensure it was flexible enough to adapt to the other styles. Taking it out now wouldn't change much of the code. I also saw this https://github.com/yola/production/issues/3061#issuecomment-193303279

snitch · 2016-04-21T22:31:32Z

✨ No lint errors found. ✨

coveralls · 2016-04-21T22:35:25Z

Changes Unknown when pulling 9de4f7b on morris/initial into * on master*.

LionOps · 2016-04-21T22:36:45Z

API.md

-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.


snitch · 2016-04-21T23:30:58Z

src/paginators/base-paginator.js

+  _rejectPage(page) {
+    const message = `Invalid page "${page}".`;
+    const error = new Error(message);
+    error.response = { detail: message }


Missing semicolon.

I think this also caused tests to break.

snitch · 2016-04-21T23:54:54Z

✨ No lint errors found. ✨

coveralls · 2016-04-21T23:57:36Z

Changes Unknown when pulling 088ced4 on morris/initial into * on master*.

LionOps · 2016-04-22T17:55:48Z

👍 (since @euwest is also reviewing, let's get a second thumbs! It's a huge PR.)

euwest · 2016-04-22T18:10:08Z

src/actions.js

+  return null;
+};
+
+export const parseResponse = function(response) {


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

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.

snitch · 2016-04-22T18:56:23Z

✨ No lint errors found. ✨

coveralls · 2016-04-22T19:05:30Z

Changes Unknown when pulling 5b1b7f3 on morris/initial into * on master*.

euwest · 2016-04-22T22:50:04Z

src/actions.js

+  const count = response.count;
+  const resultCount = response.results.length;
+
+  if (count > resultCount) {


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

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.

euwest · 2016-04-25T18:52:49Z

For better or worse, the tests themselves are also included in the coverage results.

I'm guessing that's a side effect of having the tests mixed in with the implementation files.

snitch · 2016-04-25T18:57:39Z

✨ No lint errors found. ✨

morrisallison · 2016-04-25T19:00:39Z

I'm guessing that's a side effect of having the tests mixed in with the implementation files.

Yeah, I tried a bunch of different ways and just settled on that. I don't mind.

coveralls · 2016-04-25T19:00:42Z

Changes Unknown when pulling 670abb9 on morris/initial into * on master*.

euwest · 2016-04-25T20:17:38Z

👍 let's make a ticket for fleshing out the readme and API docs. as is, they aren't helpful if you don't know anything about the module.

LionOps · 2016-04-25T22:49:14Z

Closes #1 and #2

Add initial implementation

9681a81

Morris Allison III added 3 commits March 22, 2016 14:30

Refactor module imports

9d27a83

Refactor the paginator to cache promises instead of responses

476f810

Update dependency list

c857f21

LionOps reviewed Mar 28, 2016
View reviewed changes

beck reviewed Mar 30, 2016
View reviewed changes

beck reviewed Apr 1, 2016
View reviewed changes

Merge master

e844e6c

LionOps reviewed Apr 21, 2016
View reviewed changes

Morris Allison III added 2 commits April 21, 2016 16:27

Correct grammar

632f79d

Update invalid page handling to reject an error

8dc65c7

snitch reviewed Apr 21, 2016
View reviewed changes

Add missing semicolon

088ced4

euwest reviewed Apr 22, 2016
View reviewed changes

Simplify actions

5b1b7f3

euwest reviewed Apr 22, 2016
View reviewed changes

Morris Allison III added 3 commits April 25, 2016 11:39

Change absolute modules paths to be relative

ddc43d8

Remove unneeded app-module-path dependency

3a22f42

Remove the browserify paths option

670abb9

morrisallison merged commit ed093c2 into master Apr 25, 2016

morrisallison deleted the morris/initial branch April 25, 2016 20:35

morrisallison mentioned this pull request Apr 25, 2016

Prepare release v0.1.0 #5

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial implementation #3

Add initial implementation #3

morrisallison commented Mar 22, 2016

morrisallison commented Mar 22, 2016

LionOps Mar 28, 2016

beck Mar 30, 2016

morrisallison Apr 1, 2016

LionOps commented Mar 30, 2016

beck Mar 30, 2016

morrisallison Apr 1, 2016

beck commented Apr 1, 2016

beck Apr 1, 2016

morrisallison Apr 1, 2016

snitch commented Apr 21, 2016

coveralls commented Apr 21, 2016

LionOps Apr 21, 2016

snitch Apr 21, 2016

LionOps Apr 21, 2016

snitch commented Apr 21, 2016

coveralls commented Apr 21, 2016

LionOps commented Apr 22, 2016 •

edited

Loading

euwest Apr 22, 2016

morrisallison Apr 22, 2016

snitch commented Apr 22, 2016

coveralls commented Apr 22, 2016

euwest Apr 22, 2016

morrisallison Apr 25, 2016

euwest commented Apr 25, 2016

snitch commented Apr 25, 2016

morrisallison commented Apr 25, 2016

coveralls commented Apr 25, 2016

euwest commented Apr 25, 2016

LionOps commented Apr 25, 2016



		const limit = 50;
		const style = Paginator.styles.limitOffset;

Add initial implementation #3

Add initial implementation #3

Conversation

morrisallison commented Mar 22, 2016

morrisallison commented Mar 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LionOps commented Mar 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beck commented Apr 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snitch commented Apr 21, 2016

coveralls commented Apr 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snitch commented Apr 21, 2016

coveralls commented Apr 21, 2016

LionOps commented Apr 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snitch commented Apr 22, 2016

coveralls commented Apr 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

euwest commented Apr 25, 2016

snitch commented Apr 25, 2016

morrisallison commented Apr 25, 2016

coveralls commented Apr 25, 2016

euwest commented Apr 25, 2016

LionOps commented Apr 25, 2016

LionOps commented Apr 22, 2016 •

edited

Loading