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

Document grpc-js supported channel args in readme #1996

Merged
merged 1 commit into from Jan 5, 2022

Conversation

josephharrington
Copy link
Contributor

This copies the list of supported channel arguments from PACKAGE_COMPARISON.md into the readme, and also adds a link to the package comparison doc.

resolves #1982, resolves #1983

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 27, 2021

CLA Signed

The committers are authorized under a signed CLA.

@murgatroid99
Copy link
Member

I am OK with this overall plan but I don't like how this list of supported channel options overwhelms the rest of the "Migrating from grpc" list. I would prefer to have a separate "Supported channel options" heading with this list and a brief explanation. Then the bullet point in the "Migrating from grpc" can have a brief explanation of what the user should do to migrate from grpc when they are using an unsupported channel option, including a reference to that other heading.

In order to avoid duplicating information that needs to be updated, I would prefer to also remove this information from the package comparison doc and replace it with a link to this README, saying something either about channel options in particular or API differences in general.

@josephharrington
Copy link
Contributor Author

@murgatroid99 Good points! Thanks for the feedback.

a brief explanation of what the user should do to migrate from grpc when they are using an unsupported channel option

For this, I think the best recommendation would just be to remove usage of any unsupported channel options. Can you think of anything else they should do?

@murgatroid99
Copy link
Member

I don't think that really addresses the main issue. Removing unsupported options is not necessary because grpc-js just ignores unrecognized options, and if the option ever is supported in the future, it will have the same semantics. The more important thing is for users to be aware of options that are not supported, and adjust their own code to compensate. In your own example in #1982, your problem was not solved by removing the unsupported option, but by finding and using the alternative option that provided the desired functionality.

So, maybe it could say something like this:

If you are using any channel options supported in grpc but not supported in @grpc/grpc-js, you may need to adjust your code to handle the different behavior. Refer to...

This moves the list of supported channel arguments from PACKAGE_COMPARISON.md into the readme, and also adds a link to the package comparison doc.

resolves grpc#1982, resolves grpc#1983
@josephharrington
Copy link
Contributor Author

@murgatroid99 Yep, agreed! I updated the change with the wording you suggested.

@murgatroid99 murgatroid99 merged commit ba3bd4b into grpc:master Jan 5, 2022
@josephharrington josephharrington deleted the patch-1 branch January 6, 2022 00:24
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.

grpc.lb_policy_name option no longer works in grpc-js
2 participants