Skip to content

Subscription support#220

Merged
martijnwalraven merged 25 commits intoapollographql:masterfrom
knutaa:master
Apr 4, 2018
Merged

Subscription support#220
martijnwalraven merged 25 commits intoapollographql:masterfrom
knutaa:master

Conversation

@knutaa
Copy link
Copy Markdown
Contributor

@knutaa knutaa commented Mar 5, 2018

Adding subscription support

  1. WebSocketTransport: web socket transport based on Starscream
  2. SplitNetworkTransport: allow for combined use of existing HTTPNetworkTransport and new WebSocketTransport
  3. ApolloClient: Added subscribe function

NOTE: Earlier GraphQLSubscriptionWatcher removed as not really necessary (as of now)

It is possible to instantiate the WebSocketTransport without initial connect, i.e. the request can be added in a later connect call. This allow for relevant information to be collected using the HTTP transport (query or mutation) before being added to the web socket request.

The WebSocketTransport can be used for all operations, queries and mutations as well as subscriptions.

@apollo-cla
Copy link
Copy Markdown

@knutaa: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

Comment thread Sources/Apollo/ApolloClient.swift Outdated
/// - queue: A dispatch queue on which the result handler will be called. Defaults to the main queue.
/// - resultHandler: An optional closure that is called when mutation results are available or when an error occurs.
/// - Returns: An object that can be used to cancel an in progress mutation.
public func subscribe<Query: GraphQLSubscription>(subscription: Query, queue: DispatchQueue = DispatchQueue.main, resultHandler: @escaping OperationResultHandler<Query>) -> GraphQLSubscriptionWatcher<Query> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Query type parameter should be Subscription I think.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible issue: We will try to send subscription request also through the existing HTTPNetworkTransport if the SplitNetworkTransport is not used

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, HTTPNetworkTransport should probably check for supported operation types and throw an error for subscriptions.

@@ -0,0 +1,34 @@
import Dispatch

public final class GraphQLSubscriptionWatcher<Subscription: GraphQLSubscription>: Cancellable
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the function of GraphQLSubscriptionWatcher? Couldn't subscribe be _subscribe and return the result of send directly?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GraphQLSubscriptionWatcher started out based on the query watcher.
Right now the only value add compared just returning the Cancellable is the "unsubscribe" function that may be more intuitive than "cancel" for terminating an active subscription. Apart from that it can be removed

Knut Johannessen and others added 3 commits March 5, 2018 23:16
Updates to ApolloClient based on review comments made the Subscription watcher redundant
Comment thread Sources/Apollo/WebSocketTransport.swift Outdated
private var subscribers = [String: (JSONObject?, Error?) -> Void]()
private var subscriptions : [String: String] = [:]

private let sendOperationIdentifiers: Bool
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some formatting inconsistencies above with spaces before/after colons. Perhaps it's time to include SwiftLint.

Comment thread Sources/Apollo/WebSocketTransport.swift Outdated
}
} else {
print("WebSocketTransport::unprocessed event \(text)")
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is quite big. It seems to do two things: Parse a message object from a string, and act depending on what message it is.

I think it would make sense to have the parsing logic in OperationMessage and make id, error and payload associated values to the different types to avoid if let logic here (what happens if you get a COMPLETE message but id is nil?).

It would then make the flow much easier to test and understand.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like this?

private func processMessage(socket: WebSocketClient, text: String) {

    OperationMessage.parseMessage(text) { (type,id,payload,error) in
        if let type = type {
            switch(type) {
                case OperationMessage.Types.DATA.rawValue,
                     OperationMessage.Types.ERROR.rawValue:
                    if let id = id {
                        if let responseHandler = subscribers[id] {
                            responseHandler(payload,error)
                        }
                    } else {
                        print("WebSocketTransport::unprocessed message \(text)")
                    }
                
                case OperationMessage.Types.COMPLETE.rawValue:
                    if let id = id {
                        // remove the callback if NOT a subscription
                        if subscriptions[id] == nil {
                            subscribers.removeValue(forKey: id)
                        }
                    } else {
                        print("WebSocketTransport::unprocessed message \(text)")
                    }
                    
                case OperationMessage.Types.CONNECTION_ACK.rawValue,
                     OperationMessage.Types.CONNECTION_KEEP_ALIVE.rawValue:
                    
                    acked = (type == OperationMessage.Types.CONNECTION_ACK.rawValue)
                    writeQueue()
                    
                default:
                    print("WebSocketTransport::unprocessed message \(text)")
                }
        } else {
            print("WebSocketTransport::unprocessed message \(text)")
        }
    }
}

@martijnwalraven martijnwalraven merged commit a483173 into apollographql:master Apr 4, 2018
@mikengyn
Copy link
Copy Markdown

mikengyn commented Apr 16, 2018

Has anyone tested an instance of SplitNetworkTransport? I've managed to connect via to our wss:// url, but none of my subscribe callbacks are being triggered.

I've managed to get it working on the web side of our project using apollo-link but no luck using this PR on IOS.

@knutaa
Copy link
Copy Markdown
Contributor Author

knutaa commented Apr 16, 2018

Would it be possible to share some more details? Believe others (and myself) has this working based on the merged apollo-ios master.

@mikengyn
Copy link
Copy Markdown

mikengyn commented Apr 16, 2018

@knutaa - thanks for the response!

Here's how i'm instantiating my ApolloClient and setting up my subscribe call. Performing the mutation should technically trigger the subscription call (which i've tested on the web and it works) But no luck here. Am I missing anything?

var wsEndpointURL: URL(string: "wss://placeholder.com/subscriptions")
var apollo: ApolloClient!
var websocket = WebSocketTransport(url: wsEndpointURL!)

let splitNetworkTransport = SplitNetworkTransport(httpNetworkTransport: HTTPNetworkTransport(url: endpointURL!, configuration: configuration), webSocketNetworkTransport: websocket)
apollo = ApolloClient(networkTransport: splitNetworkTransport)

 let sub = InventoryStatusSubscription()
 apollo.subscribe(subscription: sub, queue: .main) { (result, error) in
    if let result = result {
           // Callback completed - this isn't getting triggered
        }
     }

@wow-such-amazing
Copy link
Copy Markdown
Contributor

Have the same problem with no callbacks.
Tried configuration like @mikengyn

@jgavris
Copy link
Copy Markdown

jgavris commented Apr 16, 2018

Make sure your ApolloClient is not being deallocated. If apollo in the example above is just a local variable, it won't live long enough for you to get the subscription callback.

@wow-such-amazing
Copy link
Copy Markdown
Contributor

It is not the local variable. In my case it is a global one for now 🙃

@knutaa
Copy link
Copy Markdown
Contributor Author

knutaa commented Apr 17, 2018

@crabman448 @mikengyn Could you check with the test set-up used, i.e. the graphql server https://github.com/apollographql/starwars-server ?

Also make sure correct URI scheme (ws or wss).

@Kiesco08
Copy link
Copy Markdown

@mikengyn @crabman448 were you able to fix your issue? I'm in the same situation.

@knutaa
Copy link
Copy Markdown
Contributor Author

knutaa commented Apr 26, 2018

@crabman448 @mikengyn @Kiesco08 Please note that the configuration used for the HTTP network transport is not shared with the WebSocket transport!

In this code
var wsEndpointURL: URL(string: "wss://placeholder.com/subscriptions")

the webSocket transport will not use any header or payload parameters.

You would need to use either or both of the params or connectingParams from the WebSocket transport:

public init(url: URL, sendOperationIdentifiers: Bool = false, params: [String:String]? = nil, connectingParams: [String:String]? = [:])

@martijnwalraven
Copy link
Copy Markdown
Contributor

@knutaa Do you think we'll want a WebSocketTransportDelegate similar to #257 for more dynamic cases? That would allow you to easily share an authentication token between your HTTP and WebSocket connection, and refresh it when needed.

@ghost
Copy link
Copy Markdown

ghost commented May 10, 2018

hey guys how did you install apollo websocket ?

@Nickersoft
Copy link
Copy Markdown
Contributor

Is there documentation yet for this feature? Couldn't find any on the Apollo site.

@wtrocki
Copy link
Copy Markdown

wtrocki commented Aug 29, 2018

What will be the underlying server to test this. Is this going to work with default websocket server on Apollo 2.0 ?

@knutaa
Copy link
Copy Markdown
Contributor Author

knutaa commented Aug 30, 2018

@wtrocki. Suggest you have a look at the testcases (and https://github.com/apollographql/starwars-server). Regarding Apollo 2.0, I believe you should be able to do something like this (see below). The subscription path is by default the same as for httpx.

const express = require('express');
const http = require('http');
const { ApolloServer } = require('apollo-server-express');
const { graphiql } = require('apollo-server-module-graphiql');
const { typeDefs, resolvers } = require('./schema.js');

const server = new ApolloServer({
typeDefs,
resolvers,
subscriptions: {
onConnect: (connectionParams, webSocket) => {
console.log('new subscription');
}
},
playground: {
settings: {
'editor.cursorShape': 'line',
'editor.theme': 'light',
'tracing.hideTracingResponse': true
}
}
});

const app = express();
const PORT = ... ;
server.applyMiddleware({app});

const httpServer = http.createServer(app);
server.installSubscriptionHandlers(httpServer);

httpServer.listen(PORT, () => {
console.log(🚀 Server ready at http://localhost:${PORT}${server.graphqlPath})
console.log(🚀 Subscriptions ready at ws://localhost:${PORT}${server.subscriptionsPath})
})

@wtrocki
Copy link
Copy Markdown

wtrocki commented Aug 30, 2018

@knutaa Thank you so much. I will check htat

@knutaa
Copy link
Copy Markdown
Contributor Author

knutaa commented Aug 30, 2018 via email

@theMeatloaf
Copy link
Copy Markdown

I'm also not able to recieve callbacks....

I'm assigning my auth headers in the urlRequest thats used to configure the Websocket:

       let socketUrl = "wss://urlhere"

        var request = URLRequest(url: URL(string: socketUrl)!)

        for header in headers {
            request.setValue(header.value, forHTTPHeaderField: header.key)
        }

        let websocket = WebSocketTransport(request: request, sendOperationIdentifiers: false, connectingPayload:nil)

When trying the subscription, I get no error and it seems to be connected, but no call backs. Its Friday night so I'm going to throw in the towel for now, but i'll also try and spin up the server and give that example a go on Monday to make sure its not something on the server end.

@theMeatloaf
Copy link
Copy Markdown

bumping here...still not able to see subscription callbacks getting called. It would seem as though my websocket is certainly connected and I can even log the keep alive messages coming through, but I never see any actual data show up. Would this point to something wrong with my server? is there a more solid way to test this with a working server? Thanks

@theMeatloaf
Copy link
Copy Markdown

I finally got this figured out...

In order to get my server to properly authenticate, I had to pass the headers in the connection payload under a "headers" key.

I was passing them in the URLRequest object but that wasn't what my server was checking for. I use Hasura as my backend btw so check and see how your authentication needs to be passed in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.