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

Fixes the status codes ordering in typescript definitions #1279

Merged
merged 1 commit into from
Sep 19, 2022
Merged

Fixes the status codes ordering in typescript definitions #1279

merged 1 commit into from
Sep 19, 2022

Conversation

chandraaditya
Copy link
Contributor

@chandraaditya chandraaditya commented Sep 17, 2022

In the grpc-web package the status codes are in the wrong order causing them to be interpreted differently, so when comparing status codes that we get from a service it is not the same, this commit fixes that.

This ordering is causing misinterpretation of error codes:
Wrong ordering

Correct ordering as per gRPC specs:
Correct order

This fixes issue #1243

in the grpc-web package the status codes are in the wrong order causing them to be interpreted differently, so when comparing status codes that we get from a service it is not the same, this commit fixes that
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 17, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: chandraaditya / name: Chandraaditya (8a83701)

@chandraaditya chandraaditya changed the title fixes the status codes ordering in typescript definitions Fixes the status codes ordering in typescript definitions Sep 17, 2022
Copy link
Collaborator

@sampajano sampajano left a comment

Choose a reason for hiding this comment

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

Thanks so much for fixing this! I can't believe this was only caught now.. 😅

I think this was a bug introduced in #1139, where i updated StatusCode from a namespace to an enum.

I wish we could have a test to catch/verify this, but i don't think we have any e2e TS test yet. Will keep that in mind to add it later.

Thanks so much for the fix again!

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

Successfully merging this pull request may close these issues.

3 participants