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

Add Optional Metadata Parameter to Service Interface #11

Closed
ksaldana1 opened this issue Jul 22, 2017 · 10 comments
Closed

Add Optional Metadata Parameter to Service Interface #11

ksaldana1 opened this issue Jul 22, 2017 · 10 comments

Comments

@ksaldana1
Copy link
Contributor

ksaldana1 commented Jul 22, 2017

Currently, the generated service interface methods accept a single parameter as an argument.
Example:
getPage(request: logbook.getPageRequest): Observable<logbook.Page>;

gRPC allows you to pass optional parameters to service calls in the form of metadata. This information is passed through correctly at the moment due to ...args spread, however the type signature causes the compiler to complain. I could just add things such as the JWT token to the request proto definition itself, but metadata feels like the appropriate use case.

Do you think it would be appropriate for the interface service definitions to include metadata as an optional parameter?
getPage(request: logbook.getPageRequest, metadata?: any): Observable<logbook.Page>;

I can try to get a PR together if you'd like.

@kondi
Copy link
Owner

kondi commented Jul 25, 2017

Thank you for the idea, I like it!
However I really don't like the any type of metadata. The whole point of this project is to be typesafe.
I have to spend some time on this the figure out a good solution. Of course in the mean time I am open to hear any idea :) It would be much easier if we would have @types/grpc. Maybe somebody? :)

@ksaldana1
Copy link
Contributor Author

ksaldana1 commented Jul 25, 2017

I agree, the any type is less than ideal. gRPC does not include a d.ts file for its node client, so I wasn't sure what the shape of the metadata type looked like.

It seems to just be a class with a single "private" _internal_repr property and some public methods (get, set, add, remove, getMap, clone).

    const meta = new grpc.Metadata();
    meta.set('key', 'value');
    meta.set('key2', 'value2');
    console.log(meta);
 // prints { _internal_repr: { key: [ 'value' ], key2: [ 'value2' ] } }

https://grpc.io/grpc/node/grpc.Metadata.html for the full interface.
https://grpc.io/grpc/node/src_metadata.js.html#line50 for source around metadata.

@MaikelH
Copy link
Contributor

MaikelH commented Jul 27, 2017

There is some work being done on getting an a typescript definition file for gRPC, see #11020. Luckily for us that contains a definition file, I copied the metadata part below.

With that definition we can type the metadata parameter:

getPage( request: logbook.getPageRequest, metadata ?: Metadata ): Observable<logbook.Page>;

Definition copied from index.d.ts

  export class Metadata {
    /**
     * Class for storing metadata. Keys are normalized to lowercase ASCII.
     */
    constructor();

    /**
     * Sets the given value for the given key, replacing any other values associated
     * with that key. Normalizes the key.
     * @param key The key to set
     * @param value The value to set. Must be a buffer if and only if the normalized key ends with '-bin'
     */
    set(key: string, value: string | Buffer): void;

    /**
     * Adds the given value for the given key. Normalizes the key.
     * @param key The key to add to.
     * @param value The value to add. Must be a buffer if and only if the normalized key ends with '-bin'
     */
    add(key: string, value: string | Buffer): void;

    /**
     * Remove the given key and any associated values. Normalizes the key.
     * @param key The key to remove
     */
    remove(key: string): void;

    /**
     * Gets a list of all values associated with the key. Normalizes the key.
     * @param key The key to get
     * @return The values associated with that key
     */
    get(key: string): (string | Buffer)[];

    /**
     * Get a map of each key to a single associated value. This reflects the most
     * common way that people will want to see metadata.
     * @return A key/value mapping of the metadata
     */
    getMap(): { [index: string]: string | Buffer };

    /**
     * Clone the metadata object.
     * @return The new cloned object
     */
    clone(): Metadata;
  }

@kondi
Copy link
Owner

kondi commented Jul 27, 2017

Perfect, thank you!
I will follow that PR. Then we have to upgrade grpc, so #7 looks like a dependency.

@ksaldana1
Copy link
Contributor Author

@kondi The PR for gRPC Node typings is now living here--just a heads up.

@ksaldana1
Copy link
Contributor Author

My comment above is already obsolete. New PR here.

@ksaldana1
Copy link
Contributor Author

@kondi the typings file above has been merged in. Looks like it will be included in the next gRPC release (1.7).

@derolf
Copy link

derolf commented Oct 17, 2017

Hey, when will have the metadata params?

Here is a very ugly hack for the client side:

function withMetadata<Req, Res>(func: (req: Req) => rx.Observable<Res>) : (req: Req, metadata: grpc.Metadata) => rx.Observable<Res> {
    type MetaFunc = (req: Req, metadata: grpc.Metadata) => rx.Observable<Res>;
    return <MetaFunc>func;
}

Now you can do:

withMetadata(client.foo)(bar, metadata);

The reason why it works is that rxjs-grpc just passes the arguments downstream to grpc.

@ksaldana1
Copy link
Contributor Author

ksaldana1 commented Nov 9, 2017

@kondi the TypeScript typings for gRPC are now included in the latest release (1.7). Let me know if you'd like me to try to put together a PR.

@kondi
Copy link
Owner

kondi commented Mar 5, 2019

Released in v0.2.0.

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

4 participants