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

Make metadata and callback optional #389

Closed
wants to merge 2 commits into from

Conversation

shaxbee
Copy link
Contributor

@shaxbee shaxbee commented Nov 27, 2018

No description provided.

Copy link
Collaborator

@stanley-cheung stanley-cheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution. I suggest we hold off on this for now. In the gRPC Node API, the callback is mandatory. We are trying to keep the surface API as consistent as possible.

@shaxbee shaxbee force-pushed the optional_metadata_callback branch 2 times, most recently from 2c6382a to c660ddf Compare November 28, 2018 08:21
@shaxbee
Copy link
Contributor Author

shaxbee commented Nov 28, 2018

@stanley-cheung I've removed callback optionality.

@stanley-cheung
Copy link
Collaborator

stanley-cheung commented Nov 28, 2018

How does that work though? In unary calls, it's

client.sayHello(request, metadata, callback);

How do you make metadata optional but callback required?

Also unfortunately, changing abstractclientbase.js is a too much of a risk. There are some internal code that will need to make a coordinated change. I will certainly take this into consideration in a future refactor.

@shaxbee
Copy link
Contributor Author

shaxbee commented Nov 28, 2018

Optional as in it can be nullable. This is consistent with nodejs grpc package API.

Only change in abstractclientbase.js is this:

if (metadata) {
  xhr.headers.addAll(metadata);
}

@shaxbee shaxbee closed this Nov 28, 2018
loyalpartner pushed a commit to loyalpartner/grpc-web that referenced this pull request Sep 4, 2020
Bumps [webpack](https://github.com/webpack/webpack) from 4.29.4 to 4.30.0.
- [Release notes](https://github.com/webpack/webpack/releases)
- [Commits](webpack/webpack@v4.29.4...v4.30.0)

Signed-off-by: dependabot[bot] <support@dependabot.com>
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.

3 participants