Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose response status and metadata #3

Closed
bojand opened this issue Oct 5, 2017 · 6 comments
Closed

Expose response status and metadata #3

bojand opened this issue Oct 5, 2017 · 6 comments
Assignees

Comments

@bojand
Copy link
Owner

bojand commented Oct 5, 2017

Currently for unary and request stream calls there is no easy way to get metadata and status if we want those

Server:

function sayHello(call, callback) {
  const md = new grpc.Metadata()
  md.set('foo', 'bar')
  call.sendMetadata(md)

  const smd = new grpc.Metadata()
  smd.set('biz', 'baz')
  callback(null, { message: 'Hello ' + call.request.name }, smd);
}

With the core client:

const call = client.sayHello({ name: user }, function (err, response) {
  console.log('response')
  console.log(response)
});

call.on('metadata', m => {
  console.log('metadata')
  console.dir(m, {depth: 4, colors: true})
})

call.on('status', m => {
  console.log('status')
  console.dir(m, {depth: 4, colors: true})
})
metadata
Metadata { _internal_repr: { foo: [ 'bar' ] } }
response
{ message: 'Hello world' }
status
{ code: 0,
  details: 'OK',
  metadata: Metadata { _internal_repr: { biz: [ 'baz' ] } } }
@bojand bojand self-assigned this Oct 5, 2017
@bojand
Copy link
Owner Author

bojand commented Nov 22, 2017

A proposal:

From an api perspective... we could keep the present api for easy of use, simplicity, and compatibility. In addition we could introduce a new "lower-level" Request and Response API, something similar to:

Fluent Request API:

const caller = require('grpc-caller')
const PROTO_PATH = path.resolve(__dirname, './protos/helloworld.proto')
const client = caller('0.0.0.0:50051', PROTO_PATH, 'Greeter')

const req = new client.Request('sayHello', { name: 'Bob'}) // the method we wish to invoke with the params
  .setOptions({}) // any normal "call" options
  .setMetadata({'request-id': '1234'}) // request call metadata
  .responseMetadata(true) // we care about getting the response metadata, if false we won't wait for and catch the 'metadata' event
  .responseStatus(true) // we care about getting the response status, if false we won't wait for and catch the 'status' event

const res = await req.exec() // promise based, optionally take callback for callback-based API

console.log(res.response) 
// the response payload / "body"
// { message: 'Hello Bob!' }

console.log(res.metadata) 
// the response "header" metadata converted into a map for us
// { foo: 'bar' }

console.log(res.status) 
/*
the response status / trailer metadata: 
{ code: 0,
  details: 'OK',
  metadata: { biz: 'baz' } }
*/

console.log(res.call) // just the call instance itself

We could wrap the above in an simpler functional API:

const res = await client.request('sayHello', { name: 'Bob'}) 
  .setOptions({})
  .setMetadata({'request-id': '1234'})
  .responseMetadata(true)
  .responseStatus(true)
  .exec()

or callback based:

client.request('sayHello', { name: 'Bob'}) 
  .setOptions({})
  .setMetadata({'request-id': '1234'})
  .responseMetadata(true)
  .responseStatus(true)
  .exec((err, res) => {
    console.log(res.response)
})

Underneath the present "simple" API would be implemented using the more verbose API.

@yoitsro
Copy link

yoitsro commented Apr 7, 2018

Hey @bojand. First, thanks for all your gRPC work with Node. I appreciate it massively!

Second, how's progress coming along with this? I've seen you've got your req_res branch in progress and it's looking good so far!

I need access to the response metadata, hence why I'm here :)

@bojand
Copy link
Owner Author

bojand commented Apr 9, 2018

Hello, Yea I think I got it mostly working but wasn't really happy with the resulting API or something for some reason and I think I kind of abandoned it... Don't remember now heh. I'll have to return to this soon and review it with a clearer mind now and see what's up.

@bojand bojand mentioned this issue Apr 28, 2018
@bojand
Copy link
Owner Author

bojand commented Apr 28, 2018

@yoitsro I believe #11 adds the functionality. Please provide feedback on the new API. Note that now you have to explicitly add grpc dependency in your package.json. Thanks!

@yoitsro
Copy link

yoitsro commented Apr 29, 2018

@bojand Thank you so much for this! I've not had chance to check it out yet, but will do in a few days time. And yeah, it makes sense having grpc required separately. It's similar to what I do with https://github.com/yoitsro/joigoose and mongoose :)

@bojand
Copy link
Owner Author

bojand commented May 7, 2018

#11 is merged and published as 0.5.0.

@bojand bojand closed this as completed May 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants