koding / kite.js

Kite client in JavaScript
kite.koding.com
MIT License
75 stars 16 forks source link

Generalize Kite transports & deprecate ws property #40

Open usirin opened 7 years ago

usirin commented 7 years ago

Current kite client implementation is tightly coupled with websocket transform, the transport object it's using is assigned to kite.ws property. But we already have 2 different servers:

Since client implementation of these 2 servers are identical, only changing url to ws://someurl to http://someurl satisfies the kite initialization/connection. This works until we want to support another transport protocol which has different api characteristics (e.g: webrtc).

We are already accepting transformOptions and transformClass via options on Kite initialization step. I propose extending this feature with following steps:

class Kite extends Emitter {
  // ...
  connect() {
    const { url, transportOptions, transportClass } = this.options

    // first initialize the transport
    this.transport = new transportClass(url, transportOptions)

    // assign event handlers
    this.transport.addEventListener(Event.open, ...)

    // then connect to it.
    this.transport.connect()
  }
  // ...
}
import { Event } from 'kite.js/lib/constants'

declare type TransportEvent =
  | Event.OPEN
  | Event.CLOSE
  | Event.MESSAGE
  | Event.ERROR
  | Event.INFO

declare interface TransportEventResponder {
  [Event.OPEN]: () => void,
  [Event.CLOSE]: (event: Object) => void,
  [Event.MESSAGE]: ({ data: Object }) => void,
  [Event.ERROR]: (error: Object) => void,
  [Event.INFO]: (info: Object) => void
}

declare interface KiteTransport {
  uri: string;
  options: Object;
  responder: TransportEventResponder;
  constructor(uri, options);
  addEventListener(eventName: TransportEvent, handler: Function): void;
  connect();
  disconnect(reconnect: bool);
  send(message: string): void;
}
// ws-transport.js
import { KiteTransport } from 'kite.js'
import { Event } from 'kite.js/lib/constants'
import WebSocket from 'ws'

export default class WebSocketTransport extends KiteTransport {

  constructor(url, options) {
    // assigns url, and options to `this`.
    super(url, options)
    this.responder = null
  }

  connect() {
    this.responder = new WebSocket(this.url)
  }

  addEventListener(eventName, handler) {
    this.ready(() => {
      this.responder.addEventListener(eventName, handler)
    })
  }

  disconnect(reconnect) {
    if (this.responder) {
      this.responder.close()
      this.responder = null
    }

    if (reconnect) {
      this.connect()
    }
  }

  send(message) {
    if (!this.responder) {
      throw new Error(`responder is not set`)
    }
    this.responder.send(message)
  }
}

and to initialize a kite using this transport i would use it just as before:

import { Kite } from 'kite.js'
import WebSocketTransport from './ws-transport'

const kite = new Kite({
  url: 'ws://localhost',
  transportClass: WebSocketTransport
})
const kite = new Kite({
  transports: [{
    transport: WebSocketTransport,
    options: wsOptions
  }, {
    transport: SockJsTransport,
    options: sockJsOptions
  }]
})

kite.connect()

// maybe passing url with `connect()` call?
// kite.connect(someUrlComingFromSomewhereElse)

The aim here is to allow us using multiple transports without touching any of the existing kite code. I am still not sure how url would work in the scenario where we have multiple transports.


To sum up, since ws is used internal-only, (1) replacing it with a transport property can be done without introducing a breaking change. (2) extending kite api to accept multiple transports should be done once we are satisfied with the api and the way it's gonna work, therefore it implicitly depends on (1).

In the future i see several kite-server-<transport> packages exporting both Server classes and their Transport classes to be used by clients.

@gokmen thoughts?

gokmen commented 7 years ago

This sounds great, this.ws is not a great fit for sockjs usage I agree. and +1k for renaming it to transport but we need to make sure that existing users of kite doesn't rely on that property like https://github.com/koding/koding/blob/master/client/app/lib/kite/kodingkite.coffee#L59 it's a thing that can be fixed easily but there might be some other usages. Instead I think it's time for to make major changes like this in 2.0 branch.