-
Notifications
You must be signed in to change notification settings - Fork 683
grpc-js-core: more changes #128
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
Conversation
| certChain?: Buffer|null): ChannelCredentials { | ||
| verifyIsBufferOrNull(rootCerts, 'Root certificate'); | ||
| verifyIsBufferOrNull(privateKey, 'Private key'); | ||
| verifyIsBufferOrNull(certChain, 'Certificate chain'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these not redundant with the argument type annotations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TS doesn't auto-generate sentinel checks in JS, its typings are only enforced for other TS files. The credentials test depends on these checks existing, and is written in JS.
| * @param options Options used in generating the Metadata object. | ||
| */ | ||
| generateMetadata(options: {}): Promise<Metadata>; | ||
| generateMetadata(options?: {}): Promise<Metadata>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why make options optional here, but not in the implementations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops -- this was a leftover from some experimentation. I didn't intend to make this optional
| removeListener(event: 'metadata', listener: (metadata: Metadata) => void): | ||
| this; | ||
|
|
||
| addListener(event: 'status', listener: (status: StatusObject) => void): this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes to this file do not match the existing implementation, which only emits the status event for calls with response streaming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like that's not the case -- this test depends on 'status' being emitted for a unary call:
grpc-node/test/api/surface_test.js
Lines 896 to 904 in ac186ec
| it('should be present when a unary call succeeds', function(done) { | |
| var call = client.unary({error: false}, function(err, data) { | |
| assert.ifError(err); | |
| }); | |
| call.on('status', function(status) { | |
| assert.deepEqual(status.metadata.get('trailer-present'), ['yes']); | |
| done(); | |
| }); | |
| }); |
08314a9 to
18279fa
Compare
|
@murgatroid99 PTAL. The only change should be |
18279fa to
ea92b1b
Compare
grpc-js-core: unref http2 client socketThe
ClientHttp2Sessionkeeps the process alive after all requests are completed; this change works around that.grpc-js-core: emit status on client stream callsI think client stream calls are supposed to emit the
'status'event, so I added it here.grpc-js-core: fixing credentials functionsThis change promisifies
Http2Channel#connectand makes small changes to credentials-related files to pass theapi/credentials_test.jstest suite.grpc-js-core: fixes for interop testThis is a set of changes intended to allow the js client to pass the interop test suite.