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: Adding a failing regression test for wrapper types #689

Merged
merged 5 commits into from
Nov 13, 2022

Conversation

fizx
Copy link
Collaborator

@fizx fizx commented Oct 19, 2022

If a GRPC service returns a wrapper type (e.g. StringValue), everything breaks. This used to work in 1.110.2 at least. I'm investigating the cause. Feels related to #534, but the solution is quite unclear as yet.

return promise.then((data) => boolean | undefined.decode(new _m0.Reader(data)));

@fizx fizx changed the title Bug: Adding a failing regression test for wrapper types Fix: Adding a failing regression test for wrapper types Oct 19, 2022
@fizx
Copy link
Collaborator Author

fizx commented Oct 19, 2022

So #534 introduced a unique way of handling Timestamps. That incredibly complicates things and I believe should be reverted.

The problem at the moment is that while we're great at StringValue => string | undefined if the message in question is a field of another message, doing this magic on GRPC requests or responses is completely unaccounted for in the code.

Given the current state, we should default to "no magic", rather than "deeply broken magic."

@fizx fizx changed the title Fix: Adding a failing regression test for wrapper types fix: Adding a failing regression test for wrapper types Oct 19, 2022
@fizx
Copy link
Collaborator Author

fizx commented Oct 19, 2022

Also, I have no idea why the test bin generation is inconsistent with upstream. 😬

@fizx fizx requested a review from stephenh October 19, 2022 21:26
@stephenh
Copy link
Owner

stephenh commented Oct 22, 2022

Also, I have no idea why the test bin generation is inconsistent with upstream.

I haven't looked into the rest of this yet, but that's typically due to different versions of protoc on host machines. To avoid that, the yarn proto2bin target runs fixed version of protoc via docker, so you shouldn't see any .bin changes after running that.

Hopefully that works?

Ah, in #534 you mention you're using an M1; I wonder if our docker build needs to also specify the platform as x86 (which M1 would run emulated afaiu) to get the .*bin files to be exactly the same.

@stephenh
Copy link
Owner

@fizx fwiw I pushed the yarn proto2bin updates to this PR and every is clean/passes now; so thanks! Going to go ahead and merge this.

@stephenh stephenh merged commit bde2e28 into stephenh:main Nov 13, 2022
stephenh pushed a commit that referenced this pull request Nov 13, 2022
## [1.131.2](v1.131.1...v1.131.2) (2022-11-13)

### Bug Fixes

* Adding a failing regression test for wrapper types ([#689](#689)) ([bde2e28](bde2e28))
@stephenh
Copy link
Owner

🎉 This PR is included in version 1.131.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants