-
-
Notifications
You must be signed in to change notification settings - Fork 273
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
feat: request/response support in both operation and message objects #594
Closed
Closed
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Have you thought about whether you want to target this PR towards 2.x versions or 3.x? 🤔
Version 2.x
If you choose to target the 2.x version (I assume this is your intention), you will run into a few perspective issues that will need to be considered, let me try to explain it (this blog post explains the operation confusion). This perspective issue is also why at least I, have not tried to push the feature yet, as I could not figure out the meaning of
response
in this context 😅Current perspective
Let's consider the two operation keywords (
publish
andsubscribe
) and what it means when reading the spec file:From the spec perspective, it means the following:
From the spec perspective, it means the following:
Response keyword
Now, let's try to use the
response
keyword, what would it mean in conjunction with thepublish
operation keyword? The only way I see is that it means:What would it mean in conjunction with the
subscribe
operation keyword? The only way I see is that it means:As you can see operation keywords do not "match" with the
response
keyword. As in most casesrespond
makes more sense, but it switches based on what you try to define.Maybe it is not a problem, however, I can't see how this confusion won't come up, especially considering how confusing the operation keywords already are 😅
What are your thoughts here?
Version 3.x
If you choose to target 3.x, we are relying on #618 to build upon any agreed change from there 🙂 @fmvilas already included an example of how it could be achieved, just using
reply
keyword instead ofresponse
.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.
@jonaslagoni thank you for heads up, i intent to submit this against 2.x, because it's not major change, and can be ported forward to 3.x as well, but I fear that 3.x compliance among various tools and implementations is too far away for my goal, to get the RFC into production/real-live usage, as soon as possible.
Also thanks for detailed introspection in taxonomy of specification, the perspective is in my opinion:
basic client -> server scenario
basic server -> client scenario
basic multi-user scenario (eg. messaging server)
And for the linked #618, i actually prefer terms in a physics kind-of way, where
action / reaction
andrequest / response
are more clear to me thanmessage / reply
orrequest / reply
, but I'd definitely prefer consensus on the taxonomy before this gets into 2.x and/or 3.x, since it wouldn't make any sense to change the verb between major versions of specs