-
Notifications
You must be signed in to change notification settings - Fork 436
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
Configurable Transports and Extraction of the Node HTTP Transport #265
Conversation
This allows for setting That said, I like the idea, but I'd like to take a few extra steps there:
The essential of my proposal is to: So, here are my two cents. |
Sorry, I accidentally posted the comment above mid-edit. It's now ready. |
Yep all makes sense. A, B and C all feel achievable inside the scope of this PR. Let's revisit D when those have been achieved. Thanks for your perspective |
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.
I have nothing to add beyond what @MOZGIII already said :)
ts/src/transports/Transport.ts
Outdated
|
||
if (detectMozXHRSupport()) { | ||
return mozXhrRequest; | ||
return FetchReadableStreamTransport({ credentials: init.withCredentials ? 'include' : 'same-origin' }) |
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.
How do you configure this to omit
now?
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 implementation allows explicitly using transport: FetchReadableStreamTransport({ credentials: "omit" })
in the invocation/client instead of transport: HttpTransport({ /* whatever */ })
.
HttpTransport
can't support omit
because it's not in the "greatest common interface", as XMLHttpRequest
can only allow include
or same-origin
- https://xhr.spec.whatwg.org/#the-send()-method see credentials mode at (6) there.
So far it's all good. 👍 |
Idea of what can be exposted for each transport:
|
As for the proposed transport knobs; I don't think we should expose header configuration, this is already possible with gRPC metadata. |
It seems like metadata values map directly to headers. This kind of makes headers-metadata relationship strongly coupled. This implemnetation detail makes it so it's indeed no need to allow setting headers, because setting metadata already does that. |
Agreed on not adding headers; I can look into adding the additional configuration options to Fetch and adding some new test coverage. One thing worth explicitly calling out is that I have removed the 'NodeJS' transport from being automatically selected in the environment. Given issues such as #203 and #204 I would suggest that we extract the node-transport into it's own package (maybe: |
Agreed, it might break existing clients but I think node should be a separate concern. |
@MOZGIII |
@jonny-improbable I'd rather force-add them even if TypeScript doesn't support it - it's kind of providing the full implementation for the spec. TypeScript should not be a limiting factor for implementation. |
@MOZGIII thanks for the gentle encouragement, figured out I was being stupid - all good now :) |
Isn't the reason we shouldn't include them because the minimum browser we aim to support don't support these settings? The risk now is that users implement things tat aren't supported in the browser's we promise support for. We must at the very least expose this compromise to users. |
@johanbrandhorst The way I read @MOZGIII's request is that developers should be able to specify and configure a concrete / very browser specific transport if the wish to; this is the case with the For everyone else, they should use the |
More work on this PR - moved the code for the NodeHttpTransport over to https://github.com/improbable-eng/grpc-web-node-http-transport/tree/master |
@johanbrandhorst in general, when it comes to building projects, it's the user of the library who's in control of browser compatibility requirement, not the library owner. Owners, therefore, should strive for providing more flexibility, than less. |
Actually, I just noticed that the spread operator is used: grpc-web/ts/src/transports/http/fetch.ts Lines 80 to 86 in 84e10ba
This means any underlying values can be passed, which is good. It also means that type Omit<T, K extends keyof T> = Pick<T, Exclude<keyof T, K>>
export type FetchTransportInit = Omit<RequestInit, "headers" | "method" | "body" | "signal"> |
…rer that this is the GCD between XHR and Fetch.
@@ -4,5 +4,7 @@ process.env.NODE_TLS_REJECT_UNAUTHORIZED = "0"; | |||
process.env.DISABLE_CORS_TESTS = "true"; | |||
// Disable the WebSocket-based tests as they don't apply for Node environments | |||
process.env.DISABLE_WEBSOCKET_TESTS = "true"; | |||
// Although this will be set by Travis CI; it won't be set locally. | |||
process.env.BROWSER = "nodejs"; |
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.
As the comment suggest, this is required when running the tests locally; this was not an issue before because the node-http
transport was automatically detected by default; however now testRpcCombinations.ts
sniffs for this environment variable and configures the NodeHttpTransport
accordingly.
@@ -34,7 +34,6 @@ | |||
}, | |||
"devDependencies": { | |||
"@types/google-protobuf": "^3.2.5", | |||
"@types/node": "^10.7.1", |
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.
With the removal of the node-http transport from grpc-web-client - this is no longer required.
cancel() { | ||
this.options.debug && debug("XHR.abort"); | ||
this.xhr.abort(); | ||
} | ||
} | ||
|
||
export class MozChunkedArrayBufferXHR extends XHR { |
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.
Removed a tonne of duplicated code between the xhr and mozXhr files by using a classic Template Design Pattern.
Amazing work Jonny! |
@@ -60,7 +60,7 @@ Sending multiple messages and indicating that the client has finished sending ar | |||
|
|||
Most browser networking methods do not allow control over the sending of the body of the request, meaning that sending a single request message forces the finishing of sending, limiting these transports to unary or server-streaming methods only. | |||
|
|||
For transports that do allow control over the sending of the body (e.g. websockets - coming soon), the client can optionally indicate that it has finished sending. This is useful for client-streaming or bi-directional methods in which the server will send responses after receiving all client messages. Usage with unary methods is likely not necessary as server handlers will assume the client has finished sending after receiving the single expected message. | |||
For transports that do allow control over the sending of the body (e.g. websockets), the client can optionally indicate that it has finished sending. This is useful for client-streaming or bi-directional methods in which the server will send responses after receiving all client messages. Usage with unary methods is likely not necessary as server handlers will assume the client has finished sending after receiving the single expected message. |
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.
Unrelated change, but whilst I was here I wanted to fix this.
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.
LGTM, small documentation question. Would appreciate @MOZGIII's opinion as well, of course.
ts/docs/transport.md
Outdated
|
||
If none are found then an exception is thrown. | ||
will fall back to a DefaultHttpTransport which works across the vast majority of browsers with some limitations. See [Available Transports](#available-transports) |
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 doesn't look right
ts/src/transports/Transport.ts
Outdated
|
||
if (detectNodeHTTPSupport()) { | ||
return httpNodeRequest; | ||
} | ||
|
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.
When you view this file it's easy to notice a lot of empty lines - we should probably clean them up.
Have you considered using prettier?
ts/src/transports/http/http.ts
Outdated
return FetchReadableStreamTransport(fetchInit); | ||
} | ||
return XhrTransport({ withCredentials: init.withCredentials }); | ||
} |
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.
No newline at EOF - consider adding .editorconfig to this repo
There are some minor code-style relating things. Other that that everything looks good, great job! |
Thanks @MOZGIII and @johanbrandhorst |
Should we expect a version bump soon? |
TransportOptions
interfaceFetchTransportInit
src/transport
folder structure / package layoutsetDefaultTransport
docs/transport.md
Change Overview
Following on from the conversation in #261 this PR allows the user to configure the behavior of a given Transport, separate from the pre-existing
TransportOptions
interface which deals with passing request state (ie: host, url, callbacks, etc).To facilitate this change, the
transport
option passed to theunary
,invoke
andclient
functions is now expected to meet the newgrpc.TransportFactory
interface, ie: a func which can accept zero or more args and will return agrpc.Transport
instance.Note this results in a breaking change for anyone currently using the Websocket Transport; instead of specifying:
They would instead now call:
This change also spurred me to to fix #191 and #141 by extracting the 'Node HTTP' transport out from the gprc-web-client package and into a new module: grpc-web-node-http-client, which will be published to npm separately.
As one thing leads to another, extracting node-grpc-web-node-http-transport led to implement
grpc.setDefaultTransport() which allows the user to specify which
TransportFactory` should be .invoked if none is supplied in the options.Breaking Changes
transport
option passed tounary
,invoke
andclient
is now expected to by an instance ofTransport
, not a reference to a function which will return a Transport when invoked.grpc-web-client
no longer auto-selects a suitable transport in a NodeJS environment; users of grpc-web-client in a NodeJS environment will need to depend upongrpc-web-node-http-transport
and make use ofgrpc.setDefaultTranpsport()
.What's Next?
TransportOptions
interface conveys the intention of this object well and could be confusing to future developers. Might be wise to rename it, however this would be a breaking change now we've exported it!