GraphQLBundle
GraphQLBundle copied to clipboard
overblog
changed tests to expected behavior of object pageInfo.has*Page
| Q | A |
|---|---|
| Bug fix? | maybe it will be? |
| New feature? | no |
| BC breaks? | probably yes |
| Deprecations? | no |
| Tests pass? | no - intentionally |
| Documented? | no |
| Fixed tickets | #... |
| License | MIT |
Hello, I am really confused about object pageInfo when using pagination.
IMO the current implementation is wrong because e.g. hasPreviousPage is always returning false (when using fisrt and after combination) even there are some items which could be listed and provided data set to pagination contains the one more previous result for it.
I have walked through GraphQL specification and your documentation but unfortunatelly found no well described information about this.
The best informations are here which lead me to creating this PR. Besides there was a PR (#31) probably with the same trouble but it was closed with no more informations about it.
I wanted to create an issue but breaking your tests to demonstrate my expectations seems much better to me.
Could you please explain to me what am I missing if it is wrong? Otherwise I would appreciate admitting it is wrong on your side and I could probably help with fixing it.
Cheers!
Hello, is there any chance you will have time to validate this issue? Any information about it would be great! 🙏
Hello @s3tezsky, the implementation of the bundle first follow the Relay Specs. We are not against changes but to stay compatible with Relay client, we need to reference the specs section leading to changes in this PR.
Thanks @mcg-web for the reply. Actually my proposal shoul be OK with relay specs - in further reading I am getting to this paragraph (https://relay.dev/assets/files/connections-61fc54c286f0afc0b4f230f7c4b150bf.htm#sec-undefined.PageInfo.Fields) which may bahave the same as my expectation. That is the main reason why I am a bit confused it is not implemented that way. EDIT: AFAIK the linked article has no permanent URL. To be clear I am pointing to this https://github.com/facebook/relay/blob/master/website/spec/Connections.md#fields-2
Hi @mcg-web!
I have pushed a commit which fixes the behavior of PageInfo object. I have only 2 unresolved issues:
- The build is failing on non-existent service (after rebasing on master it is still failing) - I can give it a look later.
- I had changed the case when there are wrong provided fields
beforeandafter(logicaly imposible intersection) - which I belive should be handled asInvalidArgumentException.- Maybe you have any reason to hadle it somehow better?
- See https://github.com/overblog/GraphQLBundle/pull/824/commits/095e7dceff0ded710f33354b9c2dcbfe942fc286#diff-3d84dc2259d241d63802cf315062582313f70306599ca4cd983b42dae4d702edR133)
- it is not affected in tests so there remains 2 failing test so far
- I can affect a tests too when it is OK for you.
According to provided relay specification do you think this proposal could be accepted? Or is there anything suboptimal from your point of view?
As this change should be considered as BC break I am looking forward to see the first major version 🙏 !
Cheers!
Any news about this ? I'm having this issue too. I've tested the fix created by s3tezsky which is working great.
Would be great if it could be merged soon :)
