grpc / grpc-web

gRPC for Web Clients
https://grpc.io
Apache License 2.0
8.51k stars 766 forks source link

Status Codes are shown in incorrect alphabetical ordering #1243

Closed NoMan2000 closed 1 year ago

NoMan2000 commented 2 years ago

I don't know why, but the StatusCode enums are in the wrong order.

https://github.com/grpc/grpc-web/blob/1779661dded5b918cdd84a216f708b93ebf2384c/packages/grpc-web/index.d.ts#L130

That shows them sorted alphabetically, but they are not actually that way at all. The actual ordering:

StatusCode: {
          OK: 0,
          CANCELLED: 1,
          UNKNOWN: 2,
          INVALID_ARGUMENT: 3,
          DEADLINE_EXCEEDED: 4,
          NOT_FOUND: 5,
          ALREADY_EXISTS: 6,
          PERMISSION_DENIED: 7,
          UNAUTHENTICATED: 16,
          RESOURCE_EXHAUSTED: 8,
          FAILED_PRECONDITION: 9,
          ABORTED: 10,
          OUT_OF_RANGE: 11,
          UNIMPLEMENTED: 12,
          INTERNAL: 13,
          UNAVAILABLE: 14,
          DATA_LOSS: 15
        }
sampajano commented 2 years ago

Aha interesting.. thanks for pointing this out!

I'm curious if it causes any build time / runtime issues? Or any other usability issues?

Thanks!

chandraaditya commented 1 year ago

Yes it is causing a lot of issues, I did not notice this until now, but I have a check for grpcWeb.StatusCode.NOT_FOUND, it is code 8 not 5 on grpc-web, therefore when my backend is sending code 5 it is being interpreted as FAILED_PRECONDITION instead of NOT_FOUND which is the actual code.

sampajano commented 1 year ago

@chandraaditya thanks so much for your fix!

Yes it is causing a lot of issues, I did not notice this until now, but I have a check for grpcWeb.StatusCode.NOT_FOUND, it is code 8 not 5 on grpc-web, therefore when my backend is sending code 5 it is being interpreted as FAILED_PRECONDITION instead of NOT_FOUND which is the actual code.

Also thanks for the comment! Although I'm wonder if this will actually cause a runtime issue or not?

As far as i understand, if you have a check in .ts file that looks like if (statusCode == StatusCode.NOT_FOUND), similar to what we have in our Typescript demo:

https://github.com/grpc/grpc-web/blob/903601a4266bdd3db2a3cb3cad78ff6f03a5d68b/net/grpc/gateway/examples/echo/ts-example/client.ts#L84-L88

Then when tsc compiles the TS file, the same code will be generated as Javascript, and it would work. Is that right? (Or maybe an advanced tsc mode will directly output the enum number rather than StatusCode.NOT_FOUND?)

If so, then I guess this issue will be causing some confusion during understanding the error (when referring to the index.d.ts file), but not causing a runtime issue?

Thanks in advance :)

chandraaditya commented 1 year ago

Not a problem!

So I'm using NextJS and I'm not entirely sure how it compiles TS and if it does some weird stuff.

But essentially what was happening was I needed to check if the response was NOT_FOUND so I did rpcError.code == grpcWeb.StatusCode.NOT_FOUND, and that was for sure causing errors during runtime.

Because even though my server is sending the NOT_FOUND response which is error code 5, rpcError.Code was actually interpreted as FAILED_PRECONDITION which is error code 5. So during the check in the IF statement, rpcError.code is 5 on the left side, then on the right side grpcWeb.StatusCode.NOT_FOUND is actually 8 so that condition was failing and my code was never executed even though it is expected to pass.

But apart from the fact that my code was not working, I'm not 100% sure what the reason was, I do remember trying to console.log the if condition and it showing 5 and 8 which is when I switched from grpcWeb.StatusCode.NOT_FOUND to just 5 like so: rpcError.code.valueOf() === 5 And it started working.

sampajano commented 1 year ago

Oh wow thanks for the very detailed explanation! It's very interesting to know! 😃

I'm not familiar with NextJS so maybe it's somehow directly compile the TS enum into numbers (unlike the TS example we have, which pretty much generated the same JS code and was working fine for us.)

We'll definitely keep this difference in mind going forward, and will be careful about future enums!

Thanks a lot for your report and code fix! 😃