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

Pagination: clarify behavior of first/last calls. #735

Closed
vslinko opened this issue Jan 12, 2016 · 9 comments
Closed

Pagination: clarify behavior of first/last calls. #735

vslinko opened this issue Jan 12, 2016 · 9 comments

Comments

@vslinko
Copy link
Contributor

vslinko commented Jan 12, 2016

Copy of graphql/graphql-spec#104

I'm trying to implement pagination algorithm for my data and I don't know how to handle cases when first and last are negative.

What is your opinion about this?

ref: https://facebook.github.io/relay/graphql/connections.htm#sec-Pagination-algorithm

I think that spec should answer that question.

@josephsavona
Copy link
Contributor

I agree with @leebyron's response in the original issue - first and last must be positive integers; this should be added to the Relay connection spec and graphql-relay-js's connection helper should throw on invalid first/last values.

Any interest in submitting a PR for this?

@vslinko
Copy link
Contributor Author

vslinko commented Jan 13, 2016

I can submit PR for graphql-relay-js and I can try to submit to Relay spec, but I'm not sure about my English.

@josephsavona
Copy link
Contributor

@BerndWessels this issue is about the semenatics of first/last calls and is largely resolved. Can you move your comment to #540?

@josephsavona josephsavona changed the title Pagination algorithm is not comprehensive Pagination: clarify behavior of first/last calls. Jan 14, 2016
@kassens
Copy link
Member

kassens commented Jan 15, 2016

Is there a reason not to allow 0? This might be useful for a case where you initially show no comments on an object and after pressing "see more comments" you increase from 0 upwards.

I would think we should update the spec and tests to non-negative. Thoughts?

@vslinko
Copy link
Contributor Author

vslinko commented Jan 15, 2016

@kassens In current implementation first: 0 means "give me them all".
I think it isn't good decision and I agree with you that first: 0 could be helpful in some cases, so I can change my PRs.

@josephsavona what are you think?

@josephsavona
Copy link
Contributor

I trust @kassens ;-)

My first thought was that first:0 would be the same as @include(if:false), but there might be fields on the connection that you want to fetch other than edges. Changing the semantics of first:0 to return zero edges makes sense, but we should verify that this won't impact products before making the change. Perhaps we could start with a warning in the writer if first is zero but edges are returned.

@KyleAMathews
Copy link
Contributor

👍 to first: 0 just returning no edges. I'm an avid user of connectionFields and had been assuming I'd be able to use first: 0 when I wanted no edge results.

@dschafer
Copy link
Contributor

dschafer commented Mar 4, 2016

Facebook's GraphQL servers throw an error when it receives either zero or a negative number. Having negative numbers throw an error is clearly the right call. I can go either way on zero; I very much like the current Facebook GraphQL server behavior since sending first: 0 is almost always an indication that the client didn't need to send that query, but I don't know if we need to express an error/empty opinion in the connection spec.

ghost pushed a commit that referenced this issue Mar 10, 2016
Summary:Ref #735
Closes #740

Differential Revision: D3037226

Pulled By: yungsters

fb-gh-sync-id: d3bf960ec651c3cf20d871396790d2085aae8898
shipit-source-id: d3bf960ec651c3cf20d871396790d2085aae8898
@wincent
Copy link
Contributor

wincent commented Jan 30, 2017

With 4eae620, I believe there's nothing left to do here. Thanks to everybody for contributing to the discussion!

@wincent wincent closed this as completed Jan 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants