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

Re-add "Fix Grpc-Message decoding (#117)" #178

Merged
merged 2 commits into from
May 1, 2018

Conversation

easyCZ
Copy link
Contributor

@easyCZ easyCZ commented Apr 21, 2018

This re-adds commit 9df9c0a.

@ishitatsuyuki Had to revert the change from #117 due to a breaking build. Will use this PR going forward.

@MarcusLongmuir
Copy link
Contributor

Looks like Uncaught AssertionError: expected no message: expected '' to equal undefined thrown is being thrown on almost all test runs.

return src
}
} else {
return ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to return empty string here instead of null or undefined (undefined was the previous behaviour)?

Appreciate that this is likely just opinion, but this is a subtle API change and the reason this PR is currently failing tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

I chose an empty string because otherwise it violates the typing.

@MarcusLongmuir MarcusLongmuir merged commit 81ebe17 into master May 1, 2018
@MarcusLongmuir MarcusLongmuir deleted the grpc-message-decoding branch May 1, 2018 08:58
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