Transport options (variant B) #261

MOZGIII · 2018-10-19T07:54:39Z

See #103
This better captures the idea I poposed at #103. It's also simpler than #260 and users have to opt-in to specify custom tranport.

johanbrandhorst

This I like!

ts/src/transports/fetch.ts

MOZGIII · 2018-10-19T08:18:14Z

I noticed some tests failed - and the reason seems to be infrastructure issue, not the code issue.
Btw, speaking of tests. The testing setup used here is not trivial, and I didn't add any new tests in this PR so far. Is it ok? If not - please help me with this.

johanbrandhorst · 2018-10-19T08:21:48Z

I'm afraid to say some of the tests in our CI setup are a bit flaky, I will rerun them if there appears to be something obviously wrong. It does raise a point though, could you add a test which just shows how to use this new fetch option functionality (and test that it works!)?

johanbrandhorst · 2018-10-19T08:22:37Z

Also see #259. @jonny-improbable I think we need to get Chrome-42 out sooner rather than later.

johanbrandhorst · 2018-10-19T08:24:19Z

I've opened #262 to deal with the CI failure (I hope)

johanbrandhorst · 2018-10-19T08:29:11Z

test/ts/src/testRpcCombinations.ts

@@ -54,7 +55,8 @@ export function runWithHttp1AndHttp2(cb: (config: TestConfig) => void) {

 export function runWithSupportedTransports(cb: (transport: grpc.TransportConstructor | undefined) => void) {
  const transports: {[key: string]: grpc.TransportConstructor | undefined} = {
-    "defaultTransport": undefined
+    "defaultTransport": undefined,
+    "fetchWithOptions": fetchRequestWithOptions({}),


I assume we may need to add this key to some test cases as well?

This seems to be used by all test cases as a way to provide transport - so it sohlud do the trick. Testing setup is rather complictaed, but seems to be prepared for testing custom transports like this. Would like a comment from @MarcusLongmuir on this.

MOZGIII · 2018-10-19T08:29:37Z

(I think) I added the new API to tests. Don't really know if it's the correct way - let's see what CI does in response to this.

johanbrandhorst · 2018-10-19T08:33:28Z

The tests probably won't pass until #262 is merged regardless, but we can force this through if everything but Chrome-42 passes.

MOZGIII · 2018-10-19T09:04:51Z

What's going on with tests? They seems to be stuck - is it Travis CI that's not scheduling them to run?

johanbrandhorst · 2018-10-19T09:06:02Z

I think it's just limited to 5 concurrent runners per project, and some of the runners are taken up by my #262. Please wait :).

MOZGIII · 2018-10-19T09:12:13Z

From the previous test run: Safari now has some strange failures - https://travis-ci.org/improbable-eng/grpc-web/builds/443570174
Can you please take a look? Doesn't seem to be caused by my changes.

johanbrandhorst · 2018-10-19T09:26:20Z

I've restarted the safari tests. As you say, they shouldn't be affected by your changes.

MOZGIII · 2018-10-19T09:34:48Z

I uncovered another problem - transport implementations are not exported. I think I have to add a way to use this fetchRequestWithOptions thing from outside explicitly.

MOZGIII · 2018-10-19T10:27:37Z

And it seems I need further help with tests - there are some failures in firefox when I test my fetchWithOptions implementation - and this is probably due to firefox not properly supporting the fetch in the first place (with the older versions). We probably need to somhow skip the tests for unsupported browsers.
I think my code is ok since it's only some tests that are failing.

johanbrandhorst · 2018-10-19T10:29:09Z

If Firefox should default to MozXHR AFAIK - if we've broken that we need to fix it. Are there tests running on Firefox with fetch that shouldn't be? I'm not very familiar with the tests either I'm afraid, but you should be able to run it locally I assume?

johanbrandhorst · 2018-10-19T10:29:38Z

If you want, you can try reaching out to @MarcusLongmuir or @jonny-improbable or @easyCZ on the grpc-web slack (see README.md).

MOZGIII · 2018-10-19T10:30:16Z

I have to step out now, hopefully they'll see this PR and comment before I get back.

jonny-improbable · 2018-10-19T18:08:39Z

Hmm, I have some reservations with this approach. The main reason being that one of the goals of grpc-web is to provide an abstraction over the underlying browser technology; hence why we have both fetch and xhr based transports.

The problem with this approach is that the caller is specifying the concrete transport to use; whilst fetch will work fine in Chrome, it would fail if executed in Firefox because streaming of response bodies is currently behind a feature flag (see here).

In the past I've considered adding a credentials field to the TransportOptions object and then implementing some logic in the XhrTransport to set the withCredentials prop accordingly; however given that we now support a WebSocket transport, such a credentials option no longer applies to all transports.

My initial thinking might be to create a clear distinction between the 2 types of transport we have: 'Http' based and 'Socket' based (where the HttpTransport is an abstraction over fetch and xhr based on the capabilities of the host browser). Once this has been achieved we can have different TransportOptions for each type of transport.

A further step could be to make the DefaultTransportFactory be injectable so the user can make any 'global' transport configuration without having to configure each rpc call site.

WDYT?

MOZGIII · 2018-10-19T18:30:49Z

Well, the idea of this PR is to utilize the pre-existing ability to specify custom transport to fine-tune the underlying transport. The real goal here is to have the ability to fine-tune the transports (in particular - just the fetch transport). This brings me to the point: even if the transpord it chosen automatically, I'd like to have a way to file-tune it's internal to better fit my needs. I also like the transport autodetection, but the ability to force the usage of a specific transport and to control it would be a must have as well.
We were able to quickly test (and ditch, cause it didn't work as we expected) the setup with credentials: 'include' in our internal dev process using the custom fetch transport copied form tree. This would be much better if we could just do it directly, without doing those "smart tricks".
So, it's not just about letting setting just the credentials (though current PR just provides that) - it's about fine-tuning the transport internal implementation.

What you're proposing is all good, but providing knobs (as an opt in - for people that need those, like me when I had that particular usecase) for transport is, I'd say more important. And, since knobs for different transports will be different, a unified interface to all of the possible tweakable parameters is not required - and adding knobs is not therefore blocked by splitting Http/Stream transports.

When interface for setting internal implementation details is ready - additional logic (like unified tweaking for unified Http tranport family) can be build ot not of it; and I'd be happy to see that, but even more I'd love the ability to tweak transports as close and flexible to native APIs as possible.

johanbrandhorst · 2018-10-19T18:37:52Z

There's something to be said for carefully adding API surface though, and if we can accomplish the same thing with a longer term goal, shouldn't we try that first? If we only need to expose credentials and they can both be shared between Fetch and XHR, lets do that now.

MOZGIII · 2018-10-19T18:49:43Z

Sure, why not. Especially that this PR just does that (for fetch), and we can implement simialr thing for XHR. We can also implement a "default" transport "withOptions", that would allow passing credentials, and setting them for fetch/XHR, and either ignoring or thoring an error if it the transport picked is not fetch not XHR. Does this plan seem reasonable?

jonnyreeves · 2018-10-19T19:14:26Z

Enjoying a glass of wine, I'm having an idea. Let me see if I can make some time to code it up at the weekend.

…

On Fri, 19 Oct 2018, 19:49 MOZGIII, ***@***.***> wrote: Sure, why not. Especially that this PR just does that (for fetch), and we can implement simialr thing for XHR. We can also implement a "default" transport "withOptions", that would allow passing credentials, and setting them for fetch/XHR, and either ignoring or thoring an error if it the transport picked is not fetch not XHR. Does this plan seems reasonable? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#261 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMN-dkoeGv4gH29A3Mjwu44gt84e1wNks5umh7IgaJpZM4Xv2Vq> .

MOZGIII · 2018-10-21T14:34:03Z

Superseded by #265

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

MOZGIII added 2 commits October 19, 2018 10:49

Added support for specifying custom options for fetch transport

f462503

Added a way to construct Fetch transport with custom options

464f4d5

This was referenced Oct 19, 2018

Option to set xhr.withCredentials #103

Closed

Transport options (variant A) #260

Closed

MOZGIII changed the title ~~Added support for specifying custom options for fetch transport~~ Transport options (variant B) Oct 19, 2018

johanbrandhorst suggested changes Oct 19, 2018

View reviewed changes

ts/src/transports/fetch.ts Show resolved Hide resolved

johanbrandhorst requested a review from MarcusLongmuir October 19, 2018 08:03

johanbrandhorst approved these changes Oct 19, 2018

View reviewed changes

Testing with fetchWithOptions transport

2c058f7

johanbrandhorst reviewed Oct 19, 2018

View reviewed changes

Only test fetchWithOptions in a browser environment

2640fe0

MOZGIII added 2 commits October 19, 2018 13:22

Exposed fetchRequestWithOptions in the public API

3e3a737

Switched to using fetchRequestWithOptions from the public API in tests

4a492a4

jonny-improbable mentioned this pull request Oct 20, 2018

Configurable Transports and Extraction of the Node HTTP Transport #265

Merged

8 tasks

MOZGIII closed this Oct 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transport options (variant B) #261

Transport options (variant B) #261

MOZGIII commented Oct 19, 2018

johanbrandhorst left a comment

MOZGIII commented Oct 19, 2018 •

edited

Loading

johanbrandhorst commented Oct 19, 2018

johanbrandhorst commented Oct 19, 2018

johanbrandhorst commented Oct 19, 2018

johanbrandhorst Oct 19, 2018

MOZGIII Oct 19, 2018

MOZGIII commented Oct 19, 2018

johanbrandhorst commented Oct 19, 2018

MOZGIII commented Oct 19, 2018

johanbrandhorst commented Oct 19, 2018

MOZGIII commented Oct 19, 2018 •

edited

Loading

johanbrandhorst commented Oct 19, 2018

MOZGIII commented Oct 19, 2018

MOZGIII commented Oct 19, 2018 •

edited

Loading

johanbrandhorst commented Oct 19, 2018

johanbrandhorst commented Oct 19, 2018

MOZGIII commented Oct 19, 2018

jonny-improbable commented Oct 19, 2018

MOZGIII commented Oct 19, 2018 •

edited

Loading

johanbrandhorst commented Oct 19, 2018

MOZGIII commented Oct 19, 2018 •

edited

Loading

jonnyreeves commented Oct 19, 2018 via email

MOZGIII commented Oct 21, 2018

Transport options (variant B) #261

Transport options (variant B) #261

Conversation

MOZGIII commented Oct 19, 2018

johanbrandhorst left a comment

Choose a reason for hiding this comment

MOZGIII commented Oct 19, 2018 • edited Loading

johanbrandhorst commented Oct 19, 2018

johanbrandhorst commented Oct 19, 2018

johanbrandhorst commented Oct 19, 2018

johanbrandhorst Oct 19, 2018

Choose a reason for hiding this comment

MOZGIII Oct 19, 2018

Choose a reason for hiding this comment

MOZGIII commented Oct 19, 2018

johanbrandhorst commented Oct 19, 2018

MOZGIII commented Oct 19, 2018

johanbrandhorst commented Oct 19, 2018

MOZGIII commented Oct 19, 2018 • edited Loading

johanbrandhorst commented Oct 19, 2018

MOZGIII commented Oct 19, 2018

MOZGIII commented Oct 19, 2018 • edited Loading

johanbrandhorst commented Oct 19, 2018

johanbrandhorst commented Oct 19, 2018

MOZGIII commented Oct 19, 2018

jonny-improbable commented Oct 19, 2018

MOZGIII commented Oct 19, 2018 • edited Loading

johanbrandhorst commented Oct 19, 2018

MOZGIII commented Oct 19, 2018 • edited Loading

jonnyreeves commented Oct 19, 2018 via email

MOZGIII commented Oct 21, 2018

MOZGIII commented Oct 19, 2018 •

edited

Loading

MOZGIII commented Oct 19, 2018 •

edited

Loading

MOZGIII commented Oct 19, 2018 •

edited

Loading

MOZGIII commented Oct 19, 2018 •

edited

Loading

MOZGIII commented Oct 19, 2018 •

edited

Loading