Configurable Transports and Extraction of the Node HTTP Transport #265

jonny-improbable · 2018-10-20T18:53:40Z

Allow the user to specify the default transport
Allow the user to customise individual Transports, beyond the common TransportOptions interface
Expose the underlying Fetch and XHR based transports
Expose proposed set of configuration options in FetchTransportInit
Refactor src/transport folder structure / package layout
Remove and extract the Node-HTTP Transport into its own npm module
Test coverage for setDefaultTransport
Revisit 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 the unary, invoke and client functions is now expected to meet the new grpc.TransportFactory interface, ie: a func which can accept zero or more args and will return a grpc.Transport instance.

grpc.unary(BookService.GetBook, {
    request: getBookRequest,
    host: host,
    transport: grpc.HttpTransport({ credentials: 'include' }),
    onEnd: ( /*...*/ )
});

Note this results in a breaking change for anyone currently using the Websocket Transport; instead of specifying:

transport: gprc.WebsocketTransportFactory

They would instead now call:

transport: grpc.WebsocketTransport()

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 to unary, invoke and client is now expected to by an instance of Transport, 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 upon grpc-web-node-http-transport and make use of grpc.setDefaultTranpsport().

What's Next?

The whole npm linking stuff is disgusting and hard to maintain. I've played around with Lerna and I feel it would dramatically simplify things; however it does require us to move around a tonne of files, and I don't want to make this diff any bigger than it already is!
I don't feel that the name of the 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!

MOZGIII · 2018-10-20T19:26:54Z

This allows for setting credentials to include, but not to omit. Obviously, I know why that is, just want to point out this solutions only allows for one tiny use case to be fixed. Maybe it's worth it.
However, on the downside, to me it looks like this exposes a new API that can make it difficult to expose a low-level customization that I'd like to have available there (or it may not, depending on what API compatibility rules are followed across releases and if it'll seem reasonable to accept breaking changes).

That said, I like the idea, but I'd like to take a few extra steps there:

Make Fetch constructor take RequestInit as a second argument.

In here:

grpc-web/ts/src/transports/fetch.ts

Lines 64 to 70 in dcda21d

    
           fetch(this.options.url, { 
        
             headers: this.metadata.toHeaders(), 
        
             method: "POST", 
        
             body: msgBytes, 
        
             credentials: this.config.credentials, 
        
             signal: this.controller && this.controller.signal 
        
           }).then((res: Response) => {

Allow setting arbitrary parameters:

fetch(this.options.url, {
  credentials: "same-origin",
  ...this.config,
  headers: this.metadata.toHeaders(),
  method: "POST",
  body: msgBytes,
  signal: this.controller && this.controller.signal
})

Add a helper function to convert HttpTransportConfig format to the format Fetch accepts (a noop in this case).
At HttpTransport, based on the detected transport type, convert the common HttpTransportConfig to whatever underlying transport factory takes (in the case of Fetch that would be calling the helper function defined at (3)).

Repeat (2), (3) and (4) for every transport in a way that makes sense for that particular transport. For example, MozXhr can define it's own interface to MozXhrConfig to use as a second constructor argument:

interface MozXhrConfig {
  withCredentials?: boolean;
}

With a func to convert the HttpTransportConfig to MozXhrConfig, let HttpTransport do the transform of the "common" representation into the transport low level one.

Instead of

grpc-web/ts/src/transports/mozXhr.ts

Lines 74 to 76 in dcda21d

    
           if (this.config.credentials === "include") { 
        
             xhr.withCredentials = true; 
        
           }

we'd then have:

if (this.config.withCredentials !== undefined) {
  xhr.withCredentials = this.config.withCredentials;
}

The essential of my proposal is to:
a) accept that transports are internally different and have to be configured differently;
b) expose that and let user do the low level configuration if the situation requires;
c) move the logic of mapping the "shared, common" config options to low level ones from the transports themselves to the HttpTransport meta-factory (that itself utilizes per-transport helper functions) - rationale is transports do not need to even know about the existence of a HttpTransportConfig; as long as they expose the API to configure what can be configured, the necessary configuration can be done by a caller, and if the autodetecting HttpTransport factory is used that would be it's job to configure the underlying transport to deliver what user requested at the HttpTransportConfig (it be a sort of GCD of the configuration possibilities for the underlying transports);
d) eliminate intertwining of the transport implementations and the HttpTransport factory - the underlying transport implementations become loosely coupled, and do not have to limit their knobs to the "GCD" of all the transports' settings.

So, here are my two cents.

MOZGIII · 2018-10-20T19:49:41Z

Sorry, I accidentally posted the comment above mid-edit. It's now ready.

jonnyreeves · 2018-10-20T20:50:26Z

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

johanbrandhorst

I have nothing to add beyond what @MOZGIII already said :)

johanbrandhorst · 2018-10-21T08:12:55Z

ts/src/transports/Transport.ts

-
-  if (detectMozXHRSupport()) {
-    return mozXhrRequest;
+    return FetchReadableStreamTransport({ credentials: init.withCredentials ? 'include' : 'same-origin' })


How do you configure this to omit now?

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.

MOZGIII · 2018-10-21T14:13:11Z

So far it's all good. 👍
I'd suggest extracting HttpTransport into a separate file. It's probably a good idea to restructure transports code a bit - with a single transport per file, and probably moving the base Transport class to a upper level directory - so that only transport implementations are left at the transports dir.

MOZGIII · 2018-10-21T14:32:08Z

Idea of what can be exposted for each transport:

XMLHttpRequest:
- ~~headers - probably easier to expose as key-value pairs field rather than native XMLHttpRequest interface;~~
- withCredentials
I considered timeout, but it seems exposing it might distrupt normal gRPC execution. However I'd still expose it with a disclamer that it is to be used on one's own risk.
fetch:
- ~~headers - can be exposed but custom merging is required~~
- mode
- credentials
- cache
- redirect
- referrer
- referrerPolicy
- integrity - might not make sense, but I don't see a reason why it wouldn't work technically
- keepalive

johanbrandhorst · 2018-10-21T15:31:08Z

As for the proposed transport knobs; I don't think we should expose header configuration, this is already possible with gRPC metadata.

MOZGIII · 2018-10-21T15:44:17Z

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.
After consulting https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md I think it's ok, and can confirm no explicit support for settings headers is nesessary by upstream gRPC design.

jonny-improbable · 2018-10-21T16:42:23Z

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: improbable-eng/grpc-web-node-http-transport)? Thoughts?

johanbrandhorst · 2018-10-21T16:44:13Z

Agreed, it might break existing clients but I think node should be a separate concern.

jonny-improbable · 2018-10-21T18:21:14Z

@MOZGIII referrerPolicy, keepalive and integrity all argued with the version of TypeScript we are using so I've left those out of the implementation; we can always add them later if people need them.

MOZGIII · 2018-10-21T19:00:04Z

@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.
It could be added later, but that's an extra step - and that's I don't like.

…ions.

jonny-improbable · 2018-10-21T19:06:04Z

@MOZGIII thanks for the gentle encouragement, figured out I was being stupid - all good now :)

johanbrandhorst · 2018-10-21T19:38:18Z

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.

jonny-improbable · 2018-10-21T19:42:56Z

@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 FetchReadableStreamTransport which will basically only work in Chrome and later versions of Edge. Using this transport, the developer assumes that their end users are only using these browsers.

For everyone else, they should use the HttpTransport which provides the Greatest Common Divisor (GCD) of XHR and FetchRedableStream based transports.

jonny-improbable · 2018-10-21T19:43:25Z

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

MOZGIII · 2018-10-21T19:58:59Z

@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.
This is not true, however whne it comes to "opinitionated" libraries/frameworks - there the flexibiliy is usually explicitly reduced to enforce some particular way of doing things.
gRPC implemenations are opinionated too in a way, but I guess they should only be opinionated when it comes to following the spec. Forcing programmers to use only a particular subset of options is not as good.

MOZGIII · 2018-10-21T20:11:38Z

Actually, I just noticed that the spread operator is used:

grpc-web/ts/src/transports/http/fetch.ts

Lines 80 to 86 in 84e10ba

    
           fetch(this.options.url, { 
        
             ...this.init, 
        
             headers: this.metadata.toHeaders(), 
        
             method: "POST", 
        
             body: msgBytes, 
        
             signal: this.controller && this.controller.signal 
        
           }).then((res: Response) => {

This means any underlying values can be passed, which is good.

It also means that FetchTransportInit can be just:

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.

This reverts commit 86b3a6f.

- Fix repo link

jonny-improbable · 2018-10-25T21:28:11Z

test/ts/node-src/node.spec.ts

@@ -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";


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.

jonny-improbable · 2018-10-25T21:38:24Z

ts/package.json

@@ -34,7 +34,6 @@
  },
  "devDependencies": {
    "@types/google-protobuf": "^3.2.5",
-    "@types/node": "^10.7.1",


With the removal of the node-http transport from grpc-web-client - this is no longer required.

jonny-improbable · 2018-10-25T21:41:44Z

ts/src/transports/http/xhr.ts

  cancel() {
    this.options.debug && debug("XHR.abort");
    this.xhr.abort();
  }
 }

+export class MozChunkedArrayBufferXHR extends XHR {


Removed a tonne of duplicated code between the xhr and mozXhr files by using a classic Template Design Pattern.

johanbrandhorst · 2018-10-25T21:43:45Z

Amazing work Jonny!

jonny-improbable · 2018-10-25T21:51:28Z

ts/docs/client.md

@@ -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.


Unrelated change, but whilst I was here I wanted to fix this.

johanbrandhorst

LGTM, small documentation question. Would appreciate @MOZGIII's opinion as well, of course.

johanbrandhorst · 2018-10-26T07:38:19Z

ts/docs/transport.md


-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)


This doesn't look right

MOZGIII · 2018-10-26T12:29:29Z

ts/src/transports/Transport.ts


-  if (detectNodeHTTPSupport()) {
-    return httpNodeRequest;
-  }



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?

MOZGIII · 2018-10-26T12:30:41Z

ts/src/transports/http/http.ts

+    return FetchReadableStreamTransport(fetchInit);
+  }
+  return XhrTransport({ withCredentials: init.withCredentials });
+}


No newline at EOF - consider adding .editorconfig to this repo

MOZGIII · 2018-10-26T12:35:05Z

There are some minor code-style relating things. Other that that everything looks good, great job!

jonny-improbable · 2018-10-27T17:03:19Z

Thanks @MOZGIII and @johanbrandhorst

MOZGIII · 2018-10-27T17:14:49Z

Should we expect a version bump soon?

jonny-improbable added 2 commits October 20, 2018 19:48

WIP to allow configuration of Transports.

fbebb1f

Fix linting

dcda21d

johanbrandhorst reviewed Oct 20, 2018

View reviewed changes

Allow independent configuration of XHR and Fetch Transports.

0774e58

johanbrandhorst reviewed Oct 21, 2018

View reviewed changes

MOZGIII mentioned this pull request Oct 21, 2018

Transport options (variant B) #261

Closed

jonny-improbable added 5 commits October 21, 2018 17:53

Fix lint

17066a9

Allow the user to specify the default transport.

dbf695b

Refactor transport package

f3d41e7

Drop support for NodeJS transports.

b08e5a8

Allow extended configuration of FetchInit

f35d8de

Add support for referrerPolicy, keepalive and integrity FetchInit opt…

84e10ba

…ions.

Remove NodeJS typings as they are no longer required.

e671411

jonny-improbable added 7 commits October 22, 2018 08:32

Rename HttpTransport -> CrossBrowserHttpTransport to make it clea…

a0cab8f

…rer that this is the GCD between XHR and Fetch.

Merge branch 'master' into feature/configurable-transports

3f668d7

Move Node HTTP transport into the repo and include it in the tests.

dbe3fbd

Fix linking script

afc8caf

Fix linking

b5c274b

Trying to get linking working on travis...

ee85f3c

Give up on using npm link :D

86b3a6f

jonnyreeves mentioned this pull request Oct 25, 2018

Support running grpc-web in React Native #141

Open

jonny-improbable added 4 commits October 25, 2018 21:38

Revert "Give up on using npm link :D"

ebcb001

This reverts commit 86b3a6f.

Make NPM Linking work after praying to the correct gods.

bf08173

Remove duplicate call to link-npm-modules.sh

26b0c07

- Reinstate note on NodeJS usage in README

43ab1e9

- Fix repo link

jonny-improbable commented Oct 25, 2018

View reviewed changes

jonny-improbable added the breaking-change Will require a major version increment label Oct 25, 2018

jonny-improbable commented Oct 25, 2018

View reviewed changes

jonny-improbable changed the title ~~[WIP] Allow configuration of Transports.~~ Configurable Transports and Extraction of the Node HTTP Transport Oct 25, 2018

johanbrandhorst approved these changes Oct 26, 2018

View reviewed changes

MOZGIII reviewed Oct 26, 2018

View reviewed changes

jonny-improbable added 2 commits October 27, 2018 08:53

Rewrite transport docs.

ca16ed5

Fix style issues.

d66c8af

jonny-improbable merged commit 554a663 into master Oct 27, 2018

jonny-improbable deleted the feature/configurable-transports branch October 27, 2018 17:03

This was referenced Oct 27, 2018

Removed Node transport from automatic detection #204

Closed

Use Lerna to manage client modules #269

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configurable Transports and Extraction of the Node HTTP Transport #265

Configurable Transports and Extraction of the Node HTTP Transport #265

jonny-improbable commented Oct 20, 2018 •

edited

MOZGIII commented Oct 20, 2018 •

edited

MOZGIII commented Oct 20, 2018

jonnyreeves commented Oct 20, 2018

johanbrandhorst left a comment

johanbrandhorst Oct 21, 2018

MOZGIII Oct 21, 2018 •

edited

MOZGIII commented Oct 21, 2018

MOZGIII commented Oct 21, 2018 •

edited

johanbrandhorst commented Oct 21, 2018

MOZGIII commented Oct 21, 2018 •

edited

jonny-improbable commented Oct 21, 2018

johanbrandhorst commented Oct 21, 2018

jonny-improbable commented Oct 21, 2018

MOZGIII commented Oct 21, 2018

jonny-improbable commented Oct 21, 2018

johanbrandhorst commented Oct 21, 2018

jonny-improbable commented Oct 21, 2018

jonny-improbable commented Oct 21, 2018

MOZGIII commented Oct 21, 2018

MOZGIII commented Oct 21, 2018 •

edited

jonny-improbable Oct 25, 2018

jonny-improbable Oct 25, 2018

jonny-improbable Oct 25, 2018

johanbrandhorst commented Oct 25, 2018

jonny-improbable Oct 25, 2018

johanbrandhorst left a comment

johanbrandhorst Oct 26, 2018

MOZGIII Oct 26, 2018 •

edited

MOZGIII Oct 26, 2018

MOZGIII commented Oct 26, 2018

jonny-improbable commented Oct 27, 2018

MOZGIII commented Oct 27, 2018


		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)

Configurable Transports and Extraction of the Node HTTP Transport #265

Configurable Transports and Extraction of the Node HTTP Transport #265

Conversation

jonny-improbable commented Oct 20, 2018 • edited

Change Overview

Breaking Changes

What's Next?

MOZGIII commented Oct 20, 2018 • edited

MOZGIII commented Oct 20, 2018

jonnyreeves commented Oct 20, 2018

johanbrandhorst left a comment

Choose a reason for hiding this comment

johanbrandhorst Oct 21, 2018

Choose a reason for hiding this comment

MOZGIII Oct 21, 2018 • edited

Choose a reason for hiding this comment

MOZGIII commented Oct 21, 2018

MOZGIII commented Oct 21, 2018 • edited

johanbrandhorst commented Oct 21, 2018

MOZGIII commented Oct 21, 2018 • edited

jonny-improbable commented Oct 21, 2018

johanbrandhorst commented Oct 21, 2018

jonny-improbable commented Oct 21, 2018

MOZGIII commented Oct 21, 2018

jonny-improbable commented Oct 21, 2018

johanbrandhorst commented Oct 21, 2018

jonny-improbable commented Oct 21, 2018

jonny-improbable commented Oct 21, 2018

MOZGIII commented Oct 21, 2018

MOZGIII commented Oct 21, 2018 • edited

jonny-improbable Oct 25, 2018

Choose a reason for hiding this comment

jonny-improbable Oct 25, 2018

Choose a reason for hiding this comment

jonny-improbable Oct 25, 2018

Choose a reason for hiding this comment

johanbrandhorst commented Oct 25, 2018

jonny-improbable Oct 25, 2018

Choose a reason for hiding this comment

johanbrandhorst left a comment

Choose a reason for hiding this comment

johanbrandhorst Oct 26, 2018

Choose a reason for hiding this comment

MOZGIII Oct 26, 2018 • edited

Choose a reason for hiding this comment

MOZGIII Oct 26, 2018

Choose a reason for hiding this comment

MOZGIII commented Oct 26, 2018

jonny-improbable commented Oct 27, 2018

MOZGIII commented Oct 27, 2018

jonny-improbable commented Oct 20, 2018 •

edited

MOZGIII commented Oct 20, 2018 •

edited

MOZGIII Oct 21, 2018 •

edited

MOZGIII commented Oct 21, 2018 •

edited

MOZGIII commented Oct 21, 2018 •

edited

MOZGIII commented Oct 21, 2018 •

edited

MOZGIII Oct 26, 2018 •

edited