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 streaming methods to PromiseClient #301

Conversation

juanjoDiaz
Copy link
Contributor

Closes #300

@juanjoDiaz juanjoDiaz force-pushed the add_streaming_methods_to_promiseclient branch 2 times, most recently from af80f65 to 656f408 Compare October 1, 2018 17:26
@stanley-cheung
Copy link
Collaborator

Sorry, there seems to be some problem of the test suite reporting back the test result, but the tests suite did failed. Here are the relevant error logs:

ERROR in ./echo_grpc_web_pb.js 284:6
Module parse failed: Identifier 'methodInfo_ServerStreamingEcho' has already been declared (284:6)
You may need an appropriate loader to handle this file type.
|  *   !proto.grpc.gateway.testing.ServerStreamingEchoResponse>}
|  */
> const methodInfo_ServerStreamingEcho = new grpc.web.AbstractClientBase.MethodInfo(
|   proto.grpc.gateway.testing.ServerStreamingEchoResponse,
|   /** @param {!proto.grpc.gateway.testing.ServerStreamingEchoRequest} request */
 @ ./client.js 21:28-60
Service 'commonjs-client' failed to build: The command '/bin/sh -c cd $EXAMPLE_DIR/commonjs-example &&   npm install &&   npm link grpc-web &&   npx webpack &&   cp echotest.html /var/www/html &&   cp dist/main.js /var/www/html/dist' returned a non-zero code: 2

@juanjoDiaz
Copy link
Contributor Author

Yeah. I modified my commit fixing that.

@juanjoDiaz
Copy link
Contributor Author

node_modules/@types/node/index.d.ts(815,38): error TS2583: Cannot find name 'Set'. Do you need to change your target library? Try changing the `lib` compiler option to es2015 or later.
Service 'ts-client' failed to build: The command '/bin/sh -c cd $EXAMPLE_DIR/ts-example &&   npm install &&   npm link grpc-web &&   tsc &&   npx webpack &&   cp echotest.html /var/www/html &&   cp dist/main.js /var/www/html/dist' returned a non-zero code: 2

Not sure if that error is related to my PR since I haven't added any Set or done anything with Sets.

@juanjoDiaz
Copy link
Contributor Author

The only Set usage I can find in the code base comes from third_party/closure-library/closure/goog/structs/set_test.js Maybe something has changed in that dependency?

@stanley-cheung
Copy link
Collaborator

Found this: DefinitelyTyped/DefinitelyTyped#29172

@stanley-cheung
Copy link
Collaborator

Looks like something recently changed. Probably I should add that "lib": ["es6"] thing and we can try rebasing this PR on top of that fix and try again. Stay tuned.

Thanks for the contribution!

@stanley-cheung
Copy link
Collaborator

The Set error should be fixed by #302. Once that's merged, please try rebasing this PR, thanks!

@stanley-cheung
Copy link
Collaborator

#302 is merged, please try rebasing this PR, thanks!

@juanjoDiaz juanjoDiaz force-pushed the add_streaming_methods_to_promiseclient branch from 656f408 to 02cfe33 Compare October 3, 2018 06:07
@juanjoDiaz
Copy link
Contributor Author

Rebased!

@stanley-cheung stanley-cheung merged commit 46df515 into grpc:master Oct 3, 2018
@mitar
Copy link
Contributor

mitar commented Oct 4, 2018

I am not sure if this works. If I try to use a streaming method on a promise client, I get:

error TypeError: Cannot read property 'serverStreaming' of undefined
    at proto.CorePromiseClient.getResults (core_grpc_web_pb.js:157)

I think this.client_.serverStreaming should be changed to this.delegateClient_.client_.serverStreaming for promise client.

@mitar
Copy link
Contributor

mitar commented Oct 5, 2018

And also this.hostname_ should be changed to this.delegateClient_.hostname_.

@juanjoDiaz
Copy link
Contributor Author

You are right!
I aimed for the smallest change and it was too small :)

I'll submit a fix.

@mitar
Copy link
Contributor

mitar commented Oct 5, 2018

Already done in #308.

@Cliftonz Cliftonz mentioned this pull request Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants