-
Notifications
You must be signed in to change notification settings - Fork 186
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
Matteo/gql #14
Matteo/gql #14
Conversation
# Conflicts: # readme.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last bit todo: tests!
- for the services, update existing ones if possible
- for the new gql api queries
async streams( parent, args, context, info ) { | ||
await validateScopes( context.scopes, 'streams:read' ) | ||
|
||
if ( args.limit && args.limit > 100 ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The discussion around max limits & default limits:
- max limits should be implemented everywhere that returns a collection (so we don't get stuff like
limit: 1000000
to potentially crash UIs - default limits: should be a wee bit more conservative, depending on the perceived user-land usage:
- e.g. commits: we can potentially display a longer list, so it can be higher (50?)
- e.g. streams: i don't think more than 20-ish? 25? would comfortably fit on a page before you need to switch to either a search or a "next page".
Another note, if we want to be lenient, we could just return the default max limit rather than throw an error, but I guess it's good to inform the dev 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, changed all the defaults to 25. Will keep the error as IMHO it's a better experience than having to open the docs and check why out of my ~123 streams only 100 are showing (happened to me in he past).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deal!
…ranches & commits. Additional changes as per PR #14.
@didimitrie should be all done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done deal. i still itch re some linting stuff, but i'll figure that out later, and then we eslint . --fix
. I can't see the test code coverage change, but doesn't matter for now - we'll pump it up later if we find critical paths not being covered
Yay then merge away. Feels so formal doing code reviews :D |
Adds userSearch and streamSearch