Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Replace argue.js usage with inline argument checking #162

Merged
merged 1 commit into from
Jan 24, 2018

Conversation

murgatroid99
Copy link
Member

This fixes #133 and #161. The error messages will be a little less helpful right now, but we can improve that later and I think it's good to get the fix out now.

The dependency on argue.js was actually removed a while ago in grpc/grpc#11707, but for some reason that change never made it out of the v1.4.x branch. So this is also correcting that error.

Copy link
Contributor

@kjin kjin left a comment

Choose a reason for hiding this comment

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

LGTM

@murgatroid99 murgatroid99 merged commit 8201554 into grpc:master Jan 24, 2018
@ntindall
Copy link

thanks for the fix @murgatroid99 !

options = {};
}
if (!((metadata instanceof Metadata) &&
(options instanceof Object) &&

Choose a reason for hiding this comment

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

This causes requests to throw when options === undefined. If this is new, intended behavior it is a breaking change from argue.js's behavior. This caused quite the headache for me and my team, as we didn't think to look to the grpc lib originally, because we felt such a breaking change would not be in a minor version bump.

For reference, we were calling the client request as such:

function makeCall(rpc, message, meta, options) {
  return new Bluebird((resolve, reject) => {
    client[rpc](message, meta, options, (err, res) => {
        if (err) {
          return reject(err);
        }
        return resolve(res);
    });
  });
}

Giving the options argument a default of = {} resolved the issue, but I don't feel like this should be necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for reporting this issue. It was also reported in #178 and a fix is in #179. We will try to publish a patch release soon after that is merged.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

grpc@1.8.0 -- cannot read property 'name' of null
4 participants