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.
Server implementation of Streamable HTTP transport #266
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
Server implementation of Streamable HTTP transport #266
Changes from all commits
cc9ae5b
970cf9d
4c7c434
e3a6109
e9caa5a
bafd9e7
b069719
e3bb99c
0013570
873f51f
268c9f7
6c1b9ba
4068e6f
251bb9d
af64a0e
8c2086e
c275a0d
f29cbe7
88dc565
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
This will throw if
this.sessionIdGenerator
is undefined - should it default to a() => undefined
stub if it isn't passed in the constructor args?(or could just change to
this.sessionId = this.sessionIdGenerator?.();
)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.
Are these ever both going to be false? (Maybe all errors?) If so we will fail to return a response at all unless I'm missing something.
If they are never both going to be false do you need both?
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.
Yes agreed,
hasOnlyNotificationsOrResponses
could probably be inlined into!hasRequests
.There should be different validation on the messages based on the spec since
is not correctly enforced as-is.
Yes - this is to satisfy:
(should actually probably be reworded to mention "accepts the input when there are no requests" or something, CTTOI)
and accords with JSON-RPC batch spec
so
res.writeHead(202).end();
below intentionally responds immediately with no body, the server can process the messages without blocking the ServerResponse.This should be clarified with some additional code comments, imo.