Node API suggestions

[HelloGrayson]

Going through the workshop today was really eye-opening.

I feel like there is alot of room for improvement on the Node TChannel API. Mainly, we need to tune the API for the 99% case, while still allowing the 1% case.

Before:

var TChannel = require('tchannel');
var myLocalIp = require('my-local-ip');

var rootChannel = TChannel();
rootChannel.listen(0, myLocalIp());
rootChannel.on('listening', function onListen() {
  console.log('got a server', rootChannel.address());
});

var TChannelThrift = rootChannel.TChannelAsThrift;

var keyChan = rootChannel.makeSubChannel({
    serviceName: process.env.USER || 'keyvalue'
});
var fs = require('fs');
var keyThrift = TChannelThrift({
    source: fs.readFileSync('./keyvalue.thrift', 'utf8')
});
var ctx = {
    store: {}
};

keyThrift.register(keyChan, 'KeyValue::get_v1', ctx, get);
keyThrift.register(keyChan, 'KeyValue::put_v1', ctx, put);

function get(context, req, head, body, cb) {
    cb(null, {
        ok: true,
        body: {
            value: context.store[body.key]
        }
    });
}
function put(context, req, head, body, cb) {
    context.store[body.key] = body.value;
    cb(null, {
        ok: true,
        body: null
    });
}

After:

var TChannel = require('tchannel');

// get tchannel and thrift ready
var tchannel = TChannel();
var keyValueThrift = TChannelThrift('./keyvalue.thrift');

// KeyValue::get endpoint
function get(request, cb) {
    // ...
}

// KeyValue::put endpoint
function put(request, cb) {
    // ...
}

// register endpoints
tchannel.thrift.register(keyValueThrift, get);
tchannel.thrift.register(keyValueThrift, put);

// start listening on autodiscovere ip & port
tchannel.listen();

These aren't literal suggestions, but I'm just trying to illustrate what an API might look like that is minimal and does what 99% of people want without too much machinery and configuration.

cc @blampe @Raynos @prashantv @jcorbin

[Raynos]

@breerly

I'm going to split your suggestions into a list of atomic changes.

• listen() will default to zero and external IP if no arguments.
• TChannelThrift should support a fileName instead of a thriftText string.
• subChannel for service registering should be optional.
• codec.register() should not need the endpoint name.
• codec.register() should not need a context.
• thrift handler should be fn(req, cb) instead of fn(ctx, req, head, body, cb)

[Raynos]

listen() should default values.

This is a great idea; we need to work out the defaults; maybe add autoListen(). I do like the semantics of listen(port) throws a "host required" exception.

[Raynos]

TChannelThrift should support a fileName instead of a thriftText string.

:+1:

[Raynos]

subChannel for service registering should be optional.

This can be done with a channel.register(serviceName, endpoint, handler) or thriftCodec.register(channel, serviceName, endpoint, handler)

Whenever you register we do need a to know serviceName & endpoint. Having a default serviceName feels a bit iffy.

[Raynos]

codec.register() should not need the endpoint name.

I don't think this is possible in JavaScript. We can't do function name introspection. Manually specifying the endpoint name seems reasonable to me.

[Raynos]

codec.register() should not need a context.

We spoke about this when we designed the as thrift layer. We decided to make it mandatory and allow room for a registerWithoutContext() method.

We really should encourage people to pass a context; i.e. a MyAppWorker instance for performance and consistency.

[Raynos]

thrift handler should be fn(req, cb) instead of fn(ctx, req, head, body, cb)

As mentioned above; we spoke about adding a registerWithoutContext()

When we designed the AsThrift layer we decided to have inreq, head, body instead of create a ThriftInRequest class. This was an optimization to avoid creating extra objects. Mutating the inreq is definitely a not cool thing.

At this point I think we could benefit from a ThriftInRequest.