Browser support #2

bergundy · 2019-02-15T17:08:59Z

There's some duplication in the client template ATM, I'll handle it later.

vogre

Thanks!

This requires a major version bump?

Missing features:

unknown performance (so I'm a bit wary of node-fetch)
alternative fetch implementation in constructor
need to extend the AbortError on timeouts to provide info on which method timed out

vogre · 2019-02-17T09:33:49Z

templates/ts/client-browser.ts

+      body: JSON.stringify(body),
+      method: 'POST',
+    });
+    const isJSON = (response.headers.get('content-type') || '').startsWith('application/json');


Good catch, I actually prefer this explicit error handling instead of the "simple" request-promise-native.

vogre · 2019-02-17T09:35:38Z

templates/ts/client-browser.ts

-        {{/throws}}
-        throw new InternalServerError(body.message);
+    } else if (!isJSON) {
+      throw new Error(`${response.status} - ${response.statusText}`);


Should we have partial body if available (e.g. slice(0,256))? Also headers might tell us the server/load-balancer the error response came from.

Let's add this in a separate commit #4

vogre · 2019-02-17T09:46:40Z

templates/ts/client-browser.ts

+  | 'window'
+  > {
+  headers?: Record<string, string>;
+}


Can options include a fetch implementation, with node-fetch being the default, so it's possible to pass a wrapped fetch for tracing similar to https://github.com/openzipkin/zipkin-js/blob/master/packages/zipkin-instrumentation-fetch/src/wrapFetch.js ?

vogre · 2019-02-17T09:49:58Z

templates/ts/client-node.ts

+// tslint:disable
+import fetch from 'node-fetch';
+import { RequestInit } from 'node-fetch';
+import AbortController from 'abort-controller';


This is not implemented on browser?

Not yet, #3

vogre · 2019-02-17T10:04:40Z

templates/ts/client-node.ts

+
+    if (mergedOptions.timeoutMs) {
+      const controller = new AbortController();
+      const timeout = setTimeout(() => controller.abort(), mergedOptions.timeoutMs);


Note that this timeout is much more crude than https://nodejs.org/api/http.html#http_request_settimeout_timeout_callback - it's a global timeout and not timeout between consecutive bytes received.

This might not matter for small messages, but highlights the lack of control of the low level request when using fetch API.

I think this is usually the expected behavior.

Both behaviors are interesting (and the interval between bytes is used by Node.js and NGINX), but I think it might be enough for now.

vogre · 2019-02-17T10:26:11Z

templates/ts/client-node.ts

@@ -0,0 +1,124 @@
+// tslint:disable
+import fetch from 'node-fetch';


Did you see any benchmark comparisons with request (and request-promise)?

As discussed F2F transition to fetch should not slow us down.

vogre · 2019-02-17T10:27:02Z

src/test/test_rpc.ts

+import { TestClient } from './client';
+
+export default async function test(client: TestClient) {
+await expect(client.bar('yay', { timeoutMs: 100 })).to.eventually.be.rejectedWith(Error, 'The user aborted a request.');


I think this is a confusing error, can we catch the AbortError and throw a TimeoutError with details "call to bar timed out"?

vogre · 2019-02-17T10:52:21Z

src/test/test_browser.ts

+          reject();
+        });
+        webpack.stdout.on('data', (buff: Buffer) => {
+          const data = buff.toString();


Ouch.. there is no nicer API for this?

I'll add a comment that this is ugly

vogre · 2019-02-17T10:59:38Z

templates/ts/client-node.ts

+  ValidationError,
+};
+
+export interface Options extends Pick<RequestInit, 'agent' | 'redirect' | 'follow' | 'compress'> {


Perhaps we shouldn't expose node-fetch options at all, and only provide our own, so that we can switch implementation easily?

I will consider but I think for now it's best not to limit our users.

bergundy

Thanks!

bergundy · 2019-02-18T08:18:01Z

src/test/test_browser.ts

+          reject();
+        });
+        webpack.stdout.on('data', (buff: Buffer) => {
+          const data = buff.toString();


I'll add a comment that this is ugly

bergundy · 2019-02-19T15:30:32Z

templates/ts/client-browser.ts

-        {{/throws}}
-        throw new InternalServerError(body.message);
+    } else if (!isJSON) {
+      throw new Error(`${response.status} - ${response.statusText}`);


Let's add this in a separate commit #4

bergundy · 2019-02-19T15:31:29Z

templates/ts/client-node.ts

@@ -0,0 +1,124 @@
+// tslint:disable
+import fetch from 'node-fetch';


As discussed F2F transition to fetch should not slow us down.

bergundy · 2019-02-19T15:34:20Z

templates/ts/client-node.ts

+  ValidationError,
+};
+
+export interface Options extends Pick<RequestInit, 'agent' | 'redirect' | 'follow' | 'compress'> {


I will consider but I think for now it's best not to limit our users.

bergundy · 2019-02-19T15:35:11Z

templates/ts/client-node.ts

+
+    if (mergedOptions.timeoutMs) {
+      const controller = new AbortController();
+      const timeout = setTimeout(() => controller.abort(), mergedOptions.timeoutMs);


I think this is usually the expected behavior.

bergundy · 2019-02-19T15:36:26Z

templates/ts/client-node.ts

+// tslint:disable
+import fetch from 'node-fetch';
+import { RequestInit } from 'node-fetch';
+import AbortController from 'abort-controller';


Not yet, #3

bergundy · 2019-02-19T15:49:34Z

@vogre PTAL

vogre

Thanks!

WDYT about passing the fetch implementation as part of options / setting it via some setFetch method for tracing?

vogre · 2019-02-19T15:54:04Z

templates/ts/client-node.ts

+
+    if (mergedOptions.timeoutMs) {
+      const controller = new AbortController();
+      const timeout = setTimeout(() => controller.abort(), mergedOptions.timeoutMs);


Both behaviors are interesting (and the interval between bytes is used by Node.js and NGINX), but I think it might be enough for now.

bergundy · 2019-02-20T07:59:10Z

Thanks!

Didn't really understand the comment about passing the fetch implementation in options, was what I did not good enough?

vogre · 2019-02-20T09:10:13Z

@bergundy: what I want is some way to provide instrumentation (e.g. counters, timings, logs, etc. for the requests). One option is instead of await fetch do await fetchImpl (when by default fetchImpl = fetch, but let users override it.

bergundy · 2019-02-20T09:28:15Z

templates/ts/client-node.ts

+    }
+
+    try {
+      const response = await (mergedOptions.fetchImplementation || fetch)(`${this.serverUrl}/{{name}}`, {


@vogre was this what you were looking for?

Separation of client-browser and client-node got me there.

bergundy added 7 commits February 15, 2019 00:33

Use node-fetch in preparation to support browser client

0cf1448

Ignore generated files

923d0e8

Support browser runtime

35a7f9b

Test browser integration

2cbf87e

Use --client / --server for CLI signature

3d0bf60

Support timeoutMs and custom headers on node fetch client

1072e98

Handle TODOs in browser client

c2a4355

bergundy requested a review from vogre February 15, 2019 17:08

vogre reviewed Feb 17, 2019

View reviewed changes

bergundy commented Feb 19, 2019

View reviewed changes

Implement CR comments

94dd9af

vogre approved these changes Feb 19, 2019

View reviewed changes

bergundy merged commit 2c7a49e into master Feb 20, 2019

bergundy deleted the browser-support branch February 20, 2019 07:59

bergundy commented Feb 20, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Browser support #2

Browser support #2

bergundy commented Feb 15, 2019

vogre left a comment

vogre Feb 17, 2019

vogre Feb 17, 2019

bergundy Feb 19, 2019

vogre Feb 17, 2019

vogre Feb 17, 2019

bergundy Feb 19, 2019

vogre Feb 17, 2019

bergundy Feb 19, 2019

vogre Feb 19, 2019

vogre Feb 17, 2019

bergundy Feb 19, 2019

vogre Feb 17, 2019

vogre Feb 17, 2019

bergundy Feb 18, 2019

vogre Feb 17, 2019

bergundy Feb 19, 2019

bergundy left a comment

bergundy Feb 18, 2019

bergundy Feb 19, 2019

bergundy Feb 19, 2019

bergundy Feb 19, 2019

bergundy Feb 19, 2019

bergundy Feb 19, 2019

bergundy commented Feb 19, 2019

vogre left a comment

vogre Feb 19, 2019

bergundy commented Feb 20, 2019

vogre commented Feb 20, 2019

bergundy Feb 20, 2019

vogre Feb 20, 2019

		@@ -0,0 +1,124 @@
		// tslint:disable
		import fetch from 'node-fetch';

Browser support #2

Browser support #2

Conversation

bergundy commented Feb 15, 2019

vogre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergundy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergundy commented Feb 19, 2019

vogre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergundy commented Feb 20, 2019

vogre commented Feb 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment