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

Addressing few issues in defer feature #2656

Merged
merged 4 commits into from May 30, 2023

Conversation

UnAfraid
Copy link
Contributor

Fixed hasNext being omitted when false, now it would only appear in the payload when there is deferred usage.
Applied @fiatja atomic patch

Here is modified version of GraphiQL playground that works with SSE https://gist.github.com/UnAfraid/c2f9ed63332c47a2d588213fdbcfd688

I did some testing in real world environment and it seems to work as intended.
I'm looking into writing some tests, but that would require implementation of SSE in the client package first correct?

And fixed hasNext to only appear in the payload when there is deferred usage
@coveralls
Copy link

Coverage Status

Coverage: 78.966% (-0.05%) from 79.014% when pulling 69be5b7 on UnAfraid:bugfix/hasNext into 8e29502 on 99designs:master.

Copy link
Collaborator

@StevenACoffman StevenACoffman left a comment

Choose a reason for hiding this comment

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

LGTM.

@StevenACoffman StevenACoffman merged commit 5c19c84 into 99designs:master May 30, 2023
18 checks passed
@StevenACoffman
Copy link
Collaborator

Thanks for this! I would love more tests, but the websocket testing story is not obvious. We are also using the unmaintained github.com/gorilla/websocket instead of the https://pkg.go.dev/nhooyr.io/websocket

@UnAfraid
Copy link
Contributor Author

I'll implement first set of tests using SSE if u don't mind

@UnAfraid UnAfraid deleted the bugfix/hasNext branch May 31, 2023 12:13
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.

None yet

3 participants