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

Adding support for limiting array length and updated next/previous logic #18

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wakemaster39
Copy link

@wakemaster39 wakemaster39 commented Feb 27, 2019

It is possible to trivially add support for limiting the size of a return object. This is important because people might ask for 10,000 items and we need to enforce a hard limit on the number of items.

From my understanding, the relay spec has also been modified to allow better handling of the hasPreviousPage/hasNextPage logic, facebook/relay#2079.

Since we have just a list here, it is very simple to have more accurate hasPreviousPage and hasNextPage logic. Should make it easier to support windowed pagination.

Seeing if this is something that would be considered before I tackle the tests.

Copy link

@sciyoshi sciyoshi left a comment

Choose a reason for hiding this comment

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

@wakemaster39 I have a very similar PR open here which addresses the same issue: https://github.com/graphql-python/graphql-relay-py/pull/14/files

Seeing yours has made me wonder which is actually correct, but in either case there's some relevant discussion there as well.

@Cito
Copy link
Member

Cito commented Jan 30, 2020

The problem with adding new features is that graphql-relay-py is a port of graphql-relay-js and we want to keep it compatible and up to date with the code and API upstream. If we add new features, and then they do it differently, we diverge. Also it becomes harder and harder to keep up to date with future changes in graphql-relay-js and maintain compatibility when we allow the code and API to diverge.

So my request is that before adding new features, we should at least discuss with the graphql-relay-js folks, e.g. by creating an issue there or even offering a PR (in this case creating a PR for JavaScript should be possible even if you're not a JavaScript expert). Also if we have two different solutions, this would be a good opportunity to discuss together with them which one is better. If we don't get feedback, we can still decide to move forward on our own account.

Let me know if you want me to start this discussion (just give me a few days) or if you want to do it.

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

Successfully merging this pull request may close these issues.

3 participants