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

Fix generic client interceptor resolution #385

Merged

Conversation

murgatroid99
Copy link
Member

This moves all of the interceptor resolution logic to the call time to avoid needing to do method name lookups. This should fix #363. I also ended up deleting a couple of helper functions in client_interceptors.js that were only used for the previous iteration of this functionality. We could probably also remove Client.prototype.$method_names, but it's used in one of the tests and I didn't feel like trying to figure out how to change it.

var interceptor_providers = options.interceptor_providers || [];
var interceptors = options.interceptors || [];
if (interceptor_providers.length && interceptors.length) {
if (_.isArray(options.interceptor_providers) && _.isArray(options.interceptors)) {
Copy link
Member

Choose a reason for hiding this comment

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

Something tells me that the original intend of the code was to basically be _.isArrayLike, by checking if it had the length property.

Copy link
Member

Choose a reason for hiding this comment

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

Also, unrelated: shouldn't we be doing something like:

if (!_.isArrayLike(options.interceptor_providers) && _.isSet(options.interceptor_providers)) {
  throw new client_interceptors.InterceptorConfigurationError('interceptor_providers needs to behave like an array')
}

Copy link
Member Author

Choose a reason for hiding this comment

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

The old code wasn't checking if they were array-like, it was checking if they were non-empty arrays. Note that that line appeared after the line that conditionally set those variables to empty arrays. Strictly speaking, it's not exactly the same behavior now, but I suspect that the old behavior of accepting two empty arrays was more error-prone.

And for now, I don't think it makes a big difference to do that additional check. There's a rabbit hole there of checking if each element is a function, and checking if each element of interceptors is actually an interceptor. I just want to get this working, especially for people who aren't using interceptors at all.

@murgatroid99 murgatroid99 merged commit 09fe69f into grpc:v1.12.x Jun 7, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Sep 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants