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 resultKey option to handle common pagination responses #10

Merged
merged 1 commit into from
Oct 13, 2016

Conversation

mirkokiefer
Copy link
Contributor

No description provided.

@AdriVanHoudt
Copy link
Contributor

@mirkokiefer This means you are paginating all your responses right?

@mirkokiefer
Copy link
Contributor Author

No, it still works for responses that don't have the responseKey.

@AdriVanHoudt
Copy link
Contributor

@mirkokiefer Well unless you somehow return a response with that key in it?

@mirkokiefer
Copy link
Contributor Author

That's true - but not sure how this could be avoided. We would need some kind of per-route config option.

Copy link
Contributor

@AdriVanHoudt AdriVanHoudt left a comment

Choose a reason for hiding this comment

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

Apart from some small request LGTM

it('Uses the result key', (done) => {

const result = {
items: [{
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a page: 1 property in the result so it mimics the use case as close as possible?

@@ -48,3 +48,30 @@ Or do `GET /users.csv`.
The header approach is prefered.

Currently the `content-disposition` header is set to `attachment;` by default since this plugin is intended for exporting purposes, if this hinders you just let us know.

To handle typical pagination responses like
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be worth noting that this will apply to all routes/export cases.
Also we might want to add a section to explain all options but that is not for the scope of this PR :P

@@ -466,4 +466,78 @@ describe('Hapi csv', () => {
});
});
});

describe('Result key (e.g. for pagination)', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to also see a test case with the resultKey option where the result does not have the key, just for good measure.

@AdriVanHoudt
Copy link
Contributor

@mirkokiefer yeah that would get difficult unless we add some option you can set per route but if this solves your use case it's fine and if needed we can refine it later on

@mirkokiefer
Copy link
Contributor Author

Added the additional test.

@AdriVanHoudt
Copy link
Contributor

@kdeclerck up to you to merge, LGTM

@kdeclerck kdeclerck merged commit 1867c2a into Salesflare:master Oct 13, 2016
@AdriVanHoudt
Copy link
Contributor

🎉 I will release a new version soon, I plan to also update the Joi version.

@AdriVanHoudt
Copy link
Contributor

@mirkokiefer I released version 2.5.0, thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants