[WIP] Add Apollo Link #186

nubbel · 2017-11-22T14:41:14Z

Also see the discussion in #173.

Terminating vs. Non-Terminating Links

ApolloLink has a concept of terminating and non-terminating links. In the Javascript implementation, the link type is determined at runtime.
I thought we could de better and determine it at compile time by using two distinct types TerminatingLink and NonTerminatingLink. This eliminates some compile time checks. For example, concat is only defined on NonTerminatingLink and returns a terminating or non-terminating link, depending on the type of the second link.
However, it makes it difficult to define a split of a terminating and a non-terminating link or vice-versa, because what should the return type be? SchrödingersLink?

Cancellation

Requests need to be cancellable, that means links need to be cancellable. I considered three options to achieve that:

Using Observables. Observables are inherently cancellable, but non-trivial to implement.
Adding a Cancelled state to Promise.
Using a cancellable object, that us to register onCancel handlers. This was inspired by an experimental Javascript feature, AbortController.
I chose the first third option and created an CancelController and CancelSignal similar to AbortController/AbortSignal in Javascript. The signal is passed in the context.
It seemed to be the less invasive solution and I actually really like the design.

Caveats

The current design has a few caveats. Most notably, links can only return one result, which renders them unsuitable for subscriptions.

Next steps

As a next step, I would like to implement an HTTPLink that pulls configuration from the context and makes a network request.
Then, I'd like to see how we could implement a RetryLink.

Instead, a protocol `GraphQLOperationBase` with no associated type has been extracted from `GraphQLOperation`.

martijnwalraven · 2017-11-23T06:53:19Z

Sources/Apollo/ApolloClient.swift

@@ -28,7 +28,7 @@ public typealias OperationResultHandler<Operation: GraphQLOperation> = (_ result

 /// The `ApolloClient` class provides the core API for Apollo. This API provides methods to fetch and watch queries, and to perform mutations.
 public class ApolloClient {
-  let networkTransport: NetworkTransport
+  let link: TerminatingLink


I'm a little confused here because the terminating link would be the last link in the chain, so if you add any other links in front of it, the link you set on the client would be non-terminating.

I agree, it could be confusing, but actually it makes quite a lot of sense.
The confusion comes from the belief that there is a chain of links, but there isn't.
The only way to combine links is by composition, which produces a new link.

Consider concatenation:
concat(NT, T) -> T and concat(NT, NT) -> NT where NT is a non-terminating link and T is a terminating link.

And split:
split(NT, NT) -> NT and split(T, T) -> T

Then there is also forced termination:
makeTerminating(NT) -> T

As you can see, no matter how many links go into the composition the end result is always a single link.

It makes sense to give the client a terminating link, because it is what produces a result in the end.
Note that this very terminating link may be a single link (e.g. HTTPLink) or a link that was composed from (multiple) other links (e.g. concat(AuthLink, split(HTTPLink, WebSocketLink))).

Does that make sense to you?

Ah, you're right. I was thinking of this as the first link in the chain, but it's function composition. If we can get this working reliably that would be great, but I don't think it's absolutely necessary to make termination part of the type system.

However, it makes it difficult to define a split of a terminating and a non-terminating link or vice-versa, because what should the return type be? SchrödingersLink?

Have you thought more about this yet?

And what does makeTerminating do?

martijnwalraven · 2017-11-23T07:00:01Z

Tests/ApolloTests/LinkTests.swift

+    let mutationLink = SetContextLink.incrementCounter(by: -1)
+
+    let splitLink = passthroughLink.split(first: queryLink, second: mutationLink) { op, _  in
+      type(of: op).rootCacheKey == "QUERY_ROOT"


We shouldn't rely on rootCacheKey to get the operation type (and probably not expose it). Instead, maybe we could have a GraphQLOperationType enum and operationType property.

I would also prefer a way to avoid having to go through type(of: op), making the properties non-static. (They can still be static in the generated code, but maybe we can define instance properties that forward to the static ones in a protocol extension.)

martijnwalraven · 2017-11-23T07:19:20Z

Sources/Apollo/ConcatLink.swift

+    self.second = second
+  }
+
+  func request<Operation>(operation: Operation, context: LinkContext) -> Promise<GraphQLResponse<Operation>> {


We really can't rely on Promise here, because links need the ability to return multiple responses. That's important for subscriptions support, but also for something like a polling link. I'd rather not implement Observable (or rely on RxSwift), but maybe a response handler callback would work? On the other hand, promises make the link code a lot easier to write, and so would observables, so maybe it is worth implementing something minimal.

Yeah, I know :(.
Observables would be nice to have and it would probably be fun to implement, but difficult to get right.
Here's another idea: Similar to the cancel signal, we could add a onNextResponse callback to the context which is called whenever a link produces a new response. What do you think?

Can you give an example of how onNextResponse would work? Would this replace returning a promise?

martijnwalraven · 2017-11-23T07:24:08Z

Sources/Apollo/GraphQLOperation.swift

@@ -1,12 +1,14 @@
-public protocol GraphQLOperation: class {
+public protocol GraphQLOperationBase {


We may want to keep the : class because it makes code using it slightly more efficient, and there is no reason to have structs conform to this protocol.

Interesting

martijnwalraven · 2017-11-23T07:51:07Z

Sources/Apollo/GraphQLOperation.swift

@@ -1,12 +1,14 @@
-public protocol GraphQLOperation: class {


Hmmm, actually, I don't think links ever need knowledge of the associated type. Here is an alternative idea:

We keep GraphQLOperation as is (so without GraphQLOperationBase), but we define a new GraphQLRequest protocol and make it the basis of links. So instead of request(operation:context:) it would now be perform(request:).

That means GraphQLRequest also contains the context, basically everything needed to perform a request. Something like:

protocol GraphQLRequest { var context: GraphQLRequestContext { get } var operationType: GraphQLOperationType { get } var requestString: String { get } var variables: GraphQLMap? { get } }

I like the idea that it makes links GraphQLRequest -> GraphQLResponse, and it cleanly differentiates between operations (reusable across requests) and requests (containing everything that is needed fora single request, including context)

We could then have a GraphQLOperationRequest class that initialises itself based on a GraphQLOperation. So similar to your LinkOperationDescriptor, but with instance properties and also containing the context.

An added benefit is that it also allows you to use links without generated code, or even without the rest of Apollo iOS by implementing something like a GraphQLDynamicRequest class that isn't initialised from a GraphQLOperation.

We do need to make GraphQLResponse depend on GraphQLRequest instead of GraphQLOperation as well, but that might be a good idea anyway because it no longer has to be generic and that way we can more easily store responses.

What do you think?

Sound great!

Glad you like it!

dangthaison91 · 2018-09-17T08:50:01Z

How is this going on @nubbel @martijnwalraven ? This is really helpful!

wtrocki · 2018-10-10T11:43:19Z

Sources/Apollo/CancelController.swift

@@ -0,0 +1,37 @@
+public final class CancelController: Cancellable {


@ziccardi seems that wrapping Cancellable is the common approach

koenpunt · 2019-01-23T16:40:36Z

Hi there, what's the status of this PR?

LeeWong · 2019-04-26T20:15:29Z

What's the status of this PR?

designatednerd · 2019-07-10T19:25:53Z

@nubbel Do you have any interest in continuing this PR? If not, I can close this out for you.

wtrocki · 2019-07-10T19:31:42Z

@designatednerd I'm so excited, but I first want to make sure. By closing, you mean resolving and merging this PR?

This PR is a really awesome way to enable people to build advanced customization on top of Apollo IOS.
There is a lot of work done already. It may require very little effort now to get it merged, but IMHO it will be the game changer that may really help the community to customize their clients without pushing some specific use cases to the core.

designatednerd · 2019-07-10T19:39:50Z

@wtrocki Closing would mean closing without merging.

We'd like to add this functionality in general, but this PR is pretty old and has a lot of outstanding comments, and I want to determine if this is something I should take the time to review here, or if that time would be better used looking into a new solution.

wtrocki · 2019-07-10T19:46:28Z

@designatednerd well understood thank you.

designatednerd · 2019-07-18T11:51:58Z

We're definitely still interested in making request and response handling more composable but at this point, this PR is super out of date and I haven't heard back from the original author for a week about whether they want to proceed. I'll close this one out but definitely keeping #173 open until I'm able to get a better line on this.

Thanks very much for your work and sorry this wound up getting neglected!

nubbel · 2019-07-19T10:02:24Z

I'm sorry I haven't been able to finish this.

@wtrocki If you intend to work on this issue and anything in this PR is useful, please feel free to reuse as much as you want.

nubbel added 4 commits November 19, 2017 23:21

Add Apollo-Link draft

27d4e2d

Get rid of LinkOperationDescriptor

1d86b65

Instead, a protocol `GraphQLOperationBase` with no associated type has been extracted from `GraphQLOperation`.

Add methods for constructing link chains

596fdb7

Add context parameter to test closure in SplitLink

4a9cb0b

nubbel mentioned this pull request Nov 22, 2017

Discussion: What would an Apollo Link implementation look like? #173

Closed

martijnwalraven reviewed Nov 23, 2017

View reviewed changes

MrAlek mentioned this pull request Dec 4, 2017

How to mock responses for UI tests? #188

Closed

martijnwalraven mentioned this pull request Dec 7, 2017

Support query batching #192

Open

martijnwalraven mentioned this pull request Feb 5, 2018

Subscriptions #60

Closed

martijnwalraven mentioned this pull request Jun 27, 2018

Add HTTPNetworkTransportDelegate and HTTPNetworkTransportInterception #297

Closed

wtrocki reviewed Oct 10, 2018

View reviewed changes

designatednerd closed this Jul 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add Apollo Link #186

[WIP] Add Apollo Link #186

nubbel commented Nov 22, 2017 •

edited

Loading

martijnwalraven Nov 23, 2017

nubbel Nov 23, 2017

martijnwalraven Nov 24, 2017

martijnwalraven Nov 23, 2017

martijnwalraven Nov 23, 2017

martijnwalraven Nov 23, 2017

nubbel Nov 23, 2017

martijnwalraven Nov 24, 2017

martijnwalraven Nov 23, 2017

nubbel Nov 23, 2017

martijnwalraven Nov 23, 2017 •

edited

Loading

nubbel Nov 23, 2017

martijnwalraven Nov 24, 2017

dangthaison91 commented Sep 17, 2018

wtrocki Oct 10, 2018 •

edited

Loading

koenpunt commented Jan 23, 2019

LeeWong commented Apr 26, 2019

designatednerd commented Jul 10, 2019

wtrocki commented Jul 10, 2019 •

edited

Loading

designatednerd commented Jul 10, 2019

wtrocki commented Jul 10, 2019

designatednerd commented Jul 18, 2019

nubbel commented Jul 19, 2019

		@@ -1,12 +1,14 @@
		public protocol GraphQLOperation: class {
		public protocol GraphQLOperationBase {

		@@ -0,0 +1,37 @@
		public final class CancelController: Cancellable {

[WIP] Add Apollo Link #186

[WIP] Add Apollo Link #186

Conversation

nubbel commented Nov 22, 2017 • edited Loading

Terminating vs. Non-Terminating Links

Cancellation

Caveats

Next steps

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

martijnwalraven Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dangthaison91 commented Sep 17, 2018

wtrocki Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

koenpunt commented Jan 23, 2019

LeeWong commented Apr 26, 2019

designatednerd commented Jul 10, 2019

wtrocki commented Jul 10, 2019 • edited Loading

designatednerd commented Jul 10, 2019

wtrocki commented Jul 10, 2019

designatednerd commented Jul 18, 2019

nubbel commented Jul 19, 2019

nubbel commented Nov 22, 2017 •

edited

Loading

martijnwalraven Nov 23, 2017 •

edited

Loading

wtrocki Oct 10, 2018 •

edited

Loading

wtrocki commented Jul 10, 2019 •

edited

Loading