Extensible visitors #27

astampoulis · 2017-11-23T16:59:25Z

This changes the base implementation of endpoints and middleware to allow for a generic visitor pattern, instead of hardcoding the specific mapServerData and mapClientData methods we used to have. That way we can separate the definition of endpoints and middleware from the definitions of visitors/"interpreters" of those types, which are defined by pattern matching on all the various cases.

As a result we could have the following package structure:

safe-api-base is where all the base definitions of endpoints are done, which basically give a well-typed alternative to using the string type for URLs (and also specifying query parameters etc.)
a number of packages that wrap popular libraries, like safe-api-fetch and safe-api-koa, which define their own interpreters/visitors of the base types, giving well-typed versions of their basic methods

This does introduce a bit of complexity, as it requires the use of higher-kinded types for typing the generic visit function correctly and maintaining all type information. (I have added an explanation in the code about how higher-kinded types are encoded in Flow. An example of such a type we have used already is $ObjMap<Keys, F> -- F is a type-level function transforming the types of values of an object, and therefore has the higher kind Type -> Type.) I do think the extra complexity is well-warranted though; we have discussed with @sleexyz that being able to define new interpreters of the endpoint constructors is necessary in order to be able to add wrapper APIs for more libraries as independent NPM packages.

Closes #21 .

astampoulis · 2017-11-23T17:00:54Z

src/client.js

+      url: `${data.url}/${url}`
+    };
+  },
+  handleQueryParams: queryParams => getData => input => {


I changed the visitor to require the input and do the fetching of query parameters from the input in place. This is an example of where the higher-kinded type is necessary (previously all our visitors where monomorphic), but will also be an essential change when we add the CaptureParam constructor.

I see...
Before:

We were building an intermediary data structure (call it ClientDataIntermediary) that at the end we would traverse with our input to end up with the desired data. For example, during the middleware composition phase, we would build up a set of queryparam keys, of which at the end we would finally traverse with the client input to build the final a dictionary of queryparam values.

After:
Rather than build up an intermediate data structure, we build up a function term of type I => ClientData, of which at the end we pass it the relevant client input and get the appropriate client data. This choice is actually the choice we went with on the hub.

I can see pros and cons with going with either choice.

Why ClientDataIntermediary
The function type makes the visitors marginally more complicated, since we have to const data = getData(input) all the time. Also, building up a data structure instead of a function type is closer to what happens with the server visitor, so we'll get some uniformity in that sense.

Why I => ClientData:
You don't have to bifuricate your logic by 1) creating an intermediary data representation and then 2) specifying how to unpack that representation to your final representation. Instead, you build up a function type that gets you your final representation directly.

sleexyz

The encoding of HKT here adds the additional constraint that each middleware constructor is defined for all input object types I: {}.

Before this PR, we could've declare a middleware constructor that was only defined for a subset of object types, i.e.

class HypotheticalMiddleware<I: { foo: string }> extends Middleware<I, I> { ... }

What do you think? Is it premature "ungeneralization" to want to be able to write middleware constructors that are defined on only a subset of object types?

sleexyz · 2017-11-25T19:29:28Z

src/client.js

+      url: `${data.url}/${url}`
+    };
+  },
+  handleQueryParams: queryParams => getData => input => {


I see...
Before:

We were building an intermediary data structure (call it ClientDataIntermediary) that at the end we would traverse with our input to end up with the desired data. For example, during the middleware composition phase, we would build up a set of queryparam keys, of which at the end we would finally traverse with the client input to build the final a dictionary of queryparam values.

After:
Rather than build up an intermediate data structure, we build up a function term of type I => ClientData, of which at the end we pass it the relevant client input and get the appropriate client data. This choice is actually the choice we went with on the hub.

I can see pros and cons with going with either choice.

Why ClientDataIntermediary
The function type makes the visitors marginally more complicated, since we have to const data = getData(input) all the time. Also, building up a data structure instead of a function type is closer to what happens with the server visitor, so we'll get some uniformity in that sense.

Why I => ClientData:
You don't have to bifuricate your logic by 1) creating an intermediary data representation and then 2) specifying how to unpack that representation to your final representation. Instead, you build up a function type that gets you your final representation directly.

astampoulis · 2017-11-25T20:15:00Z

Here are my thoughts on these:

wrt. the HypotheticalMiddleware -- I see what you're saying, but I think let's cross that bridge if we get there. Maybe there's a way to adapt the HKT encoding to do that? Do you have an example of a middleware that would behave like that? My guess would be that most middleware just 'adds' to the input type rather than impose constraints on it/do transformations on it. I do think that we won't need a large number of middleware -- we'll need something for capture params, something for specifying outputs probably (e.g. JSON encoding vs. something else), maybe something for setting a route to be GET or POST eventually.. What else do you have in mind?

(BTW -- I think we might need to revisit the name middleware. It does translate to middleware on the server, but not on the client necessarily. What do you think of "endpoint combinators"? Or something like that.)

wrt. the client visitor result type -- the change was mostly motivated because I was trying to do capture params as an example where the HKT encoding was needed, and I made the switch there, and I thought it's a switch worth doing anyway. For capture params, needing the input to generate the data is much cleaner, I think: in the alternative, the intermediary structure would need to have the url be a function, which given the input parameter would return the actual URL, with the captured parameters substituted out. (Or something of the sort). I moved the change to this PR so that the HKT encoding does get used and tested.

Since the visitor result is mostly contained within the module that does the wrapping of a package (here, isomorphic-fetch), I think we can treat it as an implementation detail and we should thus go with the representation that makes that process easier. It might make sense to write unit tests for the result of the visitor as well (e.g. to check that the right URL gets generated), but I think we should be able to do that with both representations anyway.

sleexyz

Very cool!

sleexyz · 2017-11-27T16:11:57Z

src/client.js

+  endpoint: SafeAPI.Endpoint<I, O>,
+  input: I
+): ClientData<I> {
+  const clientData: any = endpoint.visit(clientDataVisitor);


Let's see if we can get rid of this any. Maybe flow is telling us something here...

Nah, I think this is just Flow not implementing the conversion rule fully. (IIRC this is the one where I need to view a negative $Call<(X) => F X, I> as F<I>, and that only works positively in Flow.) So the direction is that we need to tell Flow something instead! 😄

sleexyz · 2017-11-27T16:15:37Z

src/index.js

 }

+export type Visitor<DataF: Function> = {|
+  handleFragment: (


Can we somehow extract these from the middleware types? Something like

handleFragment: $ExtractVisitorMethod<typeof fragment>

sleexyz · 2017-11-27T16:17:41Z

src/index.js

+
+   type F = <A>(A) => Array<A>
+   val x: $Call<F, string>
+   (x: Array<string>) is not valid and will need an intermediate cast to any


Is this actually true? I got your example to work.

https://flow.org/try/#0C4TwDgpgBAYlC8UA8BBAfAChQSgWqKAToQIYipoDcAUACYQDGANiYdAG6tQAeAXFABIAwiSZMkMADRQAzsEIBLAHYBzKtQx8CxMkjmLVabNSimoQA

Oh.. well, I take the above back! It might actually be an issue.

sleexyz · 2017-11-27T16:55:31Z

With regards to the Client visitor, I agree that the function type makes it easier to implement and we should pick whatever is easier, since that's opaque to the library user anyways.

Yeah, I think sticking to adding inputs might be a fair assumption for now. It forces our library to be more declarative. If we support arbitrary input transformations then the library might be too powerful and therefore less "analyzable".

I'm fine renaming Middleware to something else, like Combinator now that we've transposed the logic to Visitors.

sleexyz · 2017-11-27T17:11:34Z

src/index.js

  }
 }

 export interface Endpoint<I: {}, O> {
+  visit<DataF: Function>(visitor: Visitor<DataF>): $Call<DataF, {}>;


~~I'm not sure that the endpoint type is correct. visit seems to recurse down the stack at the term level, but $Call<DataF, I> will not recurse.~~

~~I think this the reason why we need any in the extractClient and extractServer.~~

nevermind I misunderstood DataF, DataF is the HKT that represents the target data structure we're mapping down to, like I => ClientData<I> or ServerData.

In that case, shouldn't it be $Call<DataF, I>?

Oh yeah -- I think I read the I: {} wrong as I = {}. This could be the issue actually!

sleexyz · 2017-11-27T21:39:01Z

src/index.js

+  handleQueryParams: <P: {}>(
+    P
+  ) => <I: {}>($Call<DataF, I>) => $Call<DataF, $Merge<I, $ExtractTypes<P>>>,
+  handleNil: () => $Call<DataF, {}>


rename to init

…-api into sl-as/extensible-visitors

sleexyz

I'm fine with making a ticket to reduce type signature duplication in index.js. Otherwise this PR looks good to me!

Antonis Stampoulis added 7 commits November 22, 2017 17:02

add payload to middleware (WIP)

c52883c

WIP add generic visitors

d88b754

finish up first working version

14c8c5e

use methods instead

7164f12

use higher-kinded visitor result type

1c663ce

clean things up a bit

29b3e54

change client visitor to require input

cef783d

astampoulis commented Nov 23, 2017

View reviewed changes

sleexyz reviewed Nov 25, 2017

View reviewed changes

sleexyz reviewed Nov 27, 2017

View reviewed changes

sleexyz suggested changes Nov 27, 2017

View reviewed changes

get rid of any

d65cf71

sleexyz reviewed Nov 27, 2017

View reviewed changes

init

2acb5fa

astampoulis force-pushed the sl-as/extensible-visitors branch from 28481f8 to 2acb5fa Compare November 27, 2017 22:41

Antonis Stampoulis and others added 4 commits November 27, 2017 17:45

adapt comments

671bf41

type alias

6929b55

Merge branch 'sl-as/extensible-visitors' of github.com:Originate/safe…

4446306

…-api into sl-as/extensible-visitors

no bind

04714ff

sleexyz added this to Code Review in recouple Nov 28, 2017

sleexyz approved these changes Nov 28, 2017

View reviewed changes

Merge branch 'master' into sl-as/extensible-visitors

1fb75d0

astampoulis mentioned this pull request Nov 28, 2017

reduce type duplication in visitor interface #30

Open

minor edits

292554e

astampoulis merged commit a8cfc21 into master Nov 29, 2017

recouple automation moved this from Code Review to Done Nov 29, 2017

astampoulis deleted the sl-as/extensible-visitors branch November 29, 2017 17:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extensible visitors #27

Extensible visitors #27

astampoulis commented Nov 23, 2017

astampoulis Nov 23, 2017

sleexyz Nov 25, 2017

sleexyz left a comment

sleexyz Nov 25, 2017

astampoulis commented Nov 25, 2017

sleexyz left a comment

sleexyz Nov 27, 2017

astampoulis Nov 27, 2017 •

edited

Loading

sleexyz Nov 27, 2017

sleexyz Nov 27, 2017

astampoulis Nov 27, 2017

sleexyz commented Nov 27, 2017

sleexyz Nov 27, 2017 •

edited

Loading

sleexyz Nov 27, 2017

astampoulis Nov 27, 2017

sleexyz Nov 27, 2017

sleexyz left a comment

Extensible visitors #27

Extensible visitors #27

Conversation

astampoulis commented Nov 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sleexyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astampoulis commented Nov 25, 2017

sleexyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astampoulis Nov 27, 2017 • edited Loading

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

sleexyz commented Nov 27, 2017

sleexyz Nov 27, 2017 • edited Loading

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

sleexyz left a comment

Choose a reason for hiding this comment

astampoulis Nov 27, 2017 •

edited

Loading

sleexyz Nov 27, 2017 •

edited

Loading