Simple layer #3

jquense · 2020-01-07T21:37:02Z

rips out RRMNL for something much smaller. I should maybe keep the socket stuff split out to avoid including socketio unnecessarily

itajaja · 2020-01-07T22:25:20Z

rips out RRMNL for something much smaller.

what's the cost/benefit of having this logic in here vs using an upstream package, if they are functionally equivalent?

jquense · 2020-01-08T03:53:16Z

The upstream package includes a lot more code/logic over what is essentially a fetch helper in an attempt to cover every possible use case. I think there is space for a something like this that is way more minimal, but covers the 80% case.

RRMNL also is badly packaged/published. We are pinned to an old version b/c the author is using the babel runtime transform, but not including babel runtime or corejs as dependencies because "it bloats the bundle". i.e. is broken by default but it's being called working as intended so IDK

jquense · 2020-01-08T13:41:58Z

also this is now better tested than the upstream one :P

jquense · 2020-01-08T14:12:03Z

I think the API for a general package we could stick in relay-tools should probably be:

import { NetworkLayer } from 'relay-runtime'
import { createFetch, createSubscribe } from 'relay-network-layer'

NetworkLayer.create(
  createFetch(opts), 
  createSubscribe(otherOpts)
)

itajaja · 2020-01-08T14:44:10Z

thanks :) and having this as a separate network layer package? does it make sense?

jquense · 2020-01-08T15:30:47Z

I was thinking of renaming this to 'relay-network-layer' and having the useAuthToken bit just be a side feature. WDT?

itajaja · 2020-01-08T16:11:34Z

might make more sense. I don't have big vesting on this so my suggestions are totally non binding :) I'll let you guys decide!

taion · 2020-01-08T19:48:45Z

did you not like my relay-fetch as a library name? https://github.com/4Catalyzer/relay-fetch

jquense · 2020-01-09T01:24:06Z

It's also good :P i already had the work here. we can put it where ever. I mostly like that relay-network-layer is available on npm and feels more canonical than RMNL, in a petty sort of way.

taion · 2020-01-09T17:02:07Z

okay, relay-network-layer does in fact sound better

jquense · 2020-01-10T16:00:54Z

gtg?

taion

I feel like this might be simpler without observables. For basic batching, you could do something like:

function fetchFn(request) {
  if (!canBatch(request)) {
    return fetchGraphql(request);
  }

  return batcher.fetch(request);
}

class Batcher {
  pendingRequests: Array<PendingRequest> | null = null;
  
  fetch(request) {
    if (!this.pendingRequests) {
      this.pendingRequests = [];
      
      setTimeout(this.executeBatch, timeoutMs);
    }
    
    return new Promise((resolve, reject) => {
      this.pendingRequests.push({
        request,
        resolve,
        reject,
      });
    });
  }
  
  executeBatch = async () => {
    const results = await fetchData(
      this.pendingRequests.map(({ request }) => request),
    );
    
    results.forEach((result, i) => {
      if (result.errors) {
        this.pendingRequests[i].reject(...);
      } else {
        this.pendingRequests[i].resolve(...);
      }
    });
  }
}

this is obviously very sketch-quality, but it seems like we can generally avoid observables here?

README.md

taion · 2020-01-10T20:17:48Z

__mocks__/socket.io-client.js

+  });
+});
+
+export default (origin, opts) => {


is this just a very small reimpl of an event emitter?

it works, i'm sure, but... could just use an event emitter :p

i know, i just copied and tweaked from some medium post on mocking socket.io

src/NetworkLayer.ts

taion · 2020-01-10T21:33:23Z

src/NetworkLayer.ts

+  batch?: boolean;
+
+  /** An authorization token sent in a header with every request */
+  token?: string;


Suggested change

token?: string;

authorizationToken?: string;

maybe? more explicit name

alternatively, maybe something like:

authorization?: string | { headerName?: string; scheme?: string; token: string; }

i think this can be removed

taion · 2020-01-10T21:44:39Z

src/createSubscribe.ts

+  socket
+    .on('connect', () => {
+      if (token) {
+        emitTransient('authenticate', token);


oh, huh, so we don't ever update auth?

i wonder if we can move to something like the apollo spec where we just send auth at connect time, then.

it's a little awkward, though, because i think in a few places we may subscribe before we auth? maybe just for telemed?

I'm actually wondering if there is a (minor) bug around here. We tear down network layers before the environment has been replaced. Envs don't really have any concept of a closed network so presumably until the environment is not used any more it'd still be trying to subscribe...

Also tho, is it actually feasible to update auth for existing subscriptions? If you switch to another user without permission would existing sub's just fail indefinitely? Obviously this doesn't really ever occur since we teardown everything on user changes. But still seems kinda sharp, really you'd only want to update auth in a refresh token sort of situation which we don't currently have.

right, this was some holdover from the old q-si stuff where you could refresh your token expiration. that hasn't been possible for quite some time now, so maybe this use case has gone away

i mean, emitTransient does nothing anyway if the subscription is closed, right? so it's not a big deal

taion · 2020-01-10T21:53:11Z

src/NetworkLayer.ts

+   * Any fetch API "init" details, this is based directly to the `fetch` call, see
+   * [MDN](https://developer.mozilla.org/en-US/docs/Web/API/Request/Request) for API details.
+   */
+  init?: RequestInit;


this API is somewhat awkward because, as constructed, it doesn't let us add extra headers or whatever

perhaps just something that accepts a custom fn with the same signature as fetch, to allow maximum flexibility?

why doesn't it allow extra headers? if you add headers in here they are used as the defaults.

oops, didn't see that you initialized headers with init?.headers

still... could be more flexible to accept a custom fetch fn that just defaults to window.fetch? or do you think it's overkill?

well, this is probably enough for now

taion · 2020-01-10T21:53:40Z

src/NetworkLayer.ts

+   *
+   * **Requires a Graphql server that understands batching"
+   */
+  batch?: boolean;


maybe:

batching?: boolean | { enabled: boolean; timeoutMs?: number; }

meh, makes it a bit harder to handle, I'd so not worth it for one additional option

alternatively, could jsut collapse this into one option that is batch timeout: false | number

sure, that works too. i got this pattern from apollo server config, where it seems... maybe fairly neat from the user pov?

src/createFetch.ts

jquense · 2020-01-13T15:40:53Z

I feel like this might be simpler without observables.

I was thinking the observables were required but I guess if we are always using the builtin Network.create it'll coerce the Promise to an observable. The real reason for the observable is the abort logic tho.

Co-Authored-By: Jimmy Jia <[email protected]>

taion · 2020-01-13T18:25:31Z

Why do you need the observable for the abort logic? Can't you just reject the promise on any sort of error?

jquense · 2020-01-13T19:26:03Z

idk how Relay converts it, the result from an abort is you complete the observable without having sent a value, with a Promise, that's a bit ambiguous

taion · 2020-01-13T19:55:29Z

okay, that makes sense. that's definitely worth adding a comment to note

Co-Authored-By: Jimmy Jia <[email protected]>

jquense · 2020-01-14T20:25:58Z

notes addressed!

taion

ok, nothing really big... just the notes here

taion · 2020-01-15T00:11:17Z

README.md

+      };
+
+  /** An authorization token sent in a header with every request */
+  token?: string;


so remove this option and just use authorization below?

and in the example above

taion · 2020-01-15T00:11:36Z

src/NetworkLayer.ts

+  batch?: boolean;
+
+  /** An authorization token sent in a header with every request */
+  token?: string;


i think this can be removed

taion · 2020-01-15T00:11:43Z

src/NetworkLayer.ts

+   * Any fetch API "init" details, this is based directly to the `fetch` call, see
+   * [MDN](https://developer.mozilla.org/en-US/docs/Web/API/Request/Request) for API details.
+   */
+  init?: RequestInit;


well, this is probably enough for now

taion · 2020-01-15T00:12:08Z

src/NetworkLayer.ts


-  constructor({
+const SimpleNetworkLayer = {
+  create({
    token,


Suggested change

token,

and just use authorization below

taion · 2020-01-15T00:12:25Z

src/NetworkLayer.ts

+    throwErrors,
+    init,
+    batch,
+    url = '/graphql',


sort these options to match their order in the docs above?

taion · 2020-01-15T00:14:48Z

src/createFetch.ts

+  init?: RequestInit;
+  throwErrors?: boolean;
+  authorization?:
+    | string


should this accept some sort of boolean or null for an explicit false?

taion · 2020-01-15T00:14:57Z

src/createSubscribe.ts

+
+export interface SubscriptionClientOptions {
+  url?: string;
+  token?: string;


perhaps this can take the same shape with authorization?: ... as in the non-subscription version?

taion · 2020-01-15T00:15:06Z

src/createSubscribe.ts

+  new (...args: any): SubscriptionClient;
+}
+
+export class SocketioSubscriptionClient implements SubscriptionClient {


Suggested change

export class SocketioSubscriptionClient implements SubscriptionClient {

export class SocketIoSubscriptionClient implements SubscriptionClient {

taion · 2020-01-15T00:15:19Z

src/createSubscribe.ts

+  close?(): void | Promise<void>;
+}
+
+interface SubscriptionClientClass {


could do Class<SubscriptionClient> with Class from utility-types

taion · 2020-01-15T00:16:18Z

src/createSubscribe.ts

+}
+
+export default function createSubscribe({
+  subscriptionClientClass = SocketioSubscriptionClient,


do you know if it's more standard for this to be called subscriptionClientClass or should it be SubscriptionClient?

im not aware of any consensus on that

jquense added 2 commits January 7, 2020 09:45

WIP

7985462

feat: add full network layer implementation

e16b637

jquense requested review from taion and AaronBuxbaum January 7, 2020 21:37

refactor api a bit

2cdab08

jquense added 2 commits January 9, 2020 18:18

Add readme

c78cc86

add entry

e7a24f4

jquense added 2 commits January 10, 2020 13:07

fix: return the correct type

828b578

tests: fix and add subscription error test

9868d69

taion reviewed Jan 10, 2020

View reviewed changes

Update README.md

59fb238

Co-Authored-By: Jimmy Jia <[email protected]>

jquense and others added 3 commits January 14, 2020 10:35

Apply suggestions from code review

0e83b6f

Co-Authored-By: Jimmy Jia <[email protected]>

address feedback

06d8f2a

feat: add throwErrors option

067fd80

jquense requested a review from taion January 14, 2020 16:32

add comment

ec6ac13

taion approved these changes Jan 15, 2020

View reviewed changes

address feedback

0251603

taion approved these changes Jan 15, 2020

View reviewed changes

jquense merged commit fc3408d into master Jan 15, 2020

jquense deleted the simple-layer branch January 15, 2020 17:35

	export class SocketioSubscriptionClient implements SubscriptionClient {
	export class SocketIoSubscriptionClient implements SubscriptionClient {

Simple layer #3

Simple layer #3

Conversation

jquense commented Jan 7, 2020

itajaja commented Jan 7, 2020

jquense commented Jan 8, 2020

jquense commented Jan 8, 2020

jquense commented Jan 8, 2020

itajaja commented Jan 8, 2020

jquense commented Jan 8, 2020

itajaja commented Jan 8, 2020

taion commented Jan 8, 2020

jquense commented Jan 9, 2020

taion commented Jan 9, 2020

jquense commented Jan 10, 2020

taion 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

jquense commented Jan 13, 2020

taion commented Jan 13, 2020

jquense commented Jan 13, 2020

taion commented Jan 13, 2020

jquense commented Jan 14, 2020

taion 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