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

Fill in missing details of the graphql-transport-ws protocol specification #342

Merged
merged 7 commits into from
Mar 25, 2022

Conversation

kristjanvalur
Copy link
Contributor

@kristjanvalur kristjanvalur commented Mar 18, 2022

this Pr makes suggested changes to clarify some aspects of the protocol specificaiton, as I understand it.. If my understanding is wrong, that is helpful too and I will be happy to amend the PR accordingly. But as it is, it is a bit ambiguous and leaves much to the implementors to decide.

I touch on the following points:

  • Clarify lifetime of IDs.
  • Clarify simultaneous Complete operations
  • Explain when unknown / completed IDs must be ignored
  • Mention that multiplexing all operations should work
  • Simplify explanation of single-result-operations, as being special cases of streaming-operations
  • Explain use of error vs next messages to report errors
  • Expand examples

See also #340 where I mention some of the above issues.

@kristjanvalur kristjanvalur changed the title Update protocol specification Fill in missing details of the graphql-transport-ws protocol specification Mar 18, 2022
@kristjanvalur kristjanvalur marked this pull request as ready for review March 18, 2022 11:17
- Clarify lifetime of IDs.
- Clarify simultaneous `Complete` operations
- Explain when unknown / completed IDs must be ignored
- Expand examples
PROTOCOL.md Outdated Show resolved Hide resolved
PROTOCOL.md Outdated Show resolved Hide resolved
PROTOCOL.md Outdated Show resolved Hide resolved
PROTOCOL.md Outdated Show resolved Hide resolved
PROTOCOL.md Outdated Show resolved Hide resolved
PROTOCOL.md Outdated Show resolved Hide resolved
PROTOCOL.md Show resolved Hide resolved
Co-authored-by: Denis Badurina <badurinadenis@gmail.com>
@kristjanvalur
Copy link
Contributor Author

I'll update my PR according to your suggestions.

@kristjanvalur
Copy link
Contributor Author

I think I have addressed all points now.

Copy link
Owner

@enisdenjo enisdenjo left a comment

Choose a reason for hiding this comment

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

Awesome, thank you very much!

@enisdenjo enisdenjo merged commit 3545c46 into enisdenjo:master Mar 25, 2022
@kristjanvalur kristjanvalur deleted the pr01 branch March 25, 2022 13:29
@enisdenjo
Copy link
Owner

🎉 This PR is included in version 5.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@enisdenjo enisdenjo added the released Has been released and published label Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Has been released and published
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants