Skip to content

fix: format of typescript definition#1463

Merged
sampajano merged 1 commit intogrpc:masterfrom
nnnnoel:master
Sep 21, 2024
Merged

fix: format of typescript definition#1463
sampajano merged 1 commit intogrpc:masterfrom
nnnnoel:master

Conversation

@nnnnoel
Copy link
Copy Markdown
Contributor

@nnnnoel nnnnoel commented Sep 13, 2024 •
edited
Loading

https://www.typescriptlang.org/docs/handbook/enums.html

syntax = "proto3";

package pokerize.api.authn;

option go_package = "gen/proto/api";

import "api/common.proto";

enum RefreshTokenResponseStatus {
  refresh_token_success = 0;
  invalid_refresh_token = 1;
  refresh_token_expired = 2;
}

message UserAccount {
  string id = 1;
  string password = 2;
}

Before

import * as jspb from 'google-protobuf'

import * as api_common_pb from '../api/common_pb'; // proto import: "api/common.proto"


export class UserAccount extends jspb.Message {
  getId(): string;
  setId(value: string): UserAccount;

  getPassword(): string;
  setPassword(value: string): UserAccount;

  serializeBinary(): Uint8Array;
  toObject(includeInstance?: boolean): UserAccount.AsObject;
  static toObject(includeInstance: boolean, msg: UserAccount): UserAccount.AsObject;
  static serializeBinaryToWriter(message: UserAccount, writer: jspb.BinaryWriter): void;
  static deserializeBinary(bytes: Uint8Array): UserAccount;
  static deserializeBinaryFromReader(message: UserAccount, reader: jspb.BinaryReader): UserAccount;
}

export namespace UserAccount {
  export type AsObject = {
    id: string,
    password: string,
  }
}

export enum RefreshTokenResponseStatus { 
  REFRESH_TOKEN_SUCCESS = 0,
  INVALID_REFRESH_TOKEN = 1,
  REFRESH_TOKEN_EXPIRED = 2,
}

After

import * as jspb from 'google-protobuf'

import * as api_common_pb from '../api/common_pb'; // proto import: "api/common.proto"


export class UserAccount extends jspb.Message {
  getId(): string;
  setId(value: string): UserAccount;

  getPassword(): string;
  setPassword(value: string): UserAccount;

  serializeBinary(): Uint8Array;
  toObject(includeInstance?: boolean): UserAccount.AsObject;
  static toObject(includeInstance: boolean, msg: UserAccount): UserAccount.AsObject;
  static serializeBinaryToWriter(message: UserAccount, writer: jspb.BinaryWriter): void;
  static deserializeBinary(bytes: Uint8Array): UserAccount;
  static deserializeBinaryFromReader(message: UserAccount, reader: jspb.BinaryReader): UserAccount;
}

export namespace UserAccount {
  export type AsObject = {
    id: string;
    password: string;
  };
}

export const enum RefreshTokenResponseStatus {
  REFRESH_TOKEN_SUCCESS = 0,
  INVALID_REFRESH_TOKEN = 1,
  REFRESH_TOKEN_EXPIRED = 2,
}

@sampajano
Copy link
Copy Markdown
Collaborator

sampajano commented Sep 13, 2024

@nnnnoel thanks for help fixing the style!!

One request: Do you mind sharing the before v.s. after view of the generated code?

Thanks!

@nnnnoel
Copy link
Copy Markdown
Contributor Author

nnnnoel commented Sep 16, 2024

@sampajano I shared by PR Comment, Please see the syntax changes and check the removed space after open bracket in enum!

@sampajano
Copy link
Copy Markdown
Collaborator

sampajano commented Sep 16, 2024

@sampajano I shared by PR Comment, Please see the syntax changes and check the removed space after open bracket in enum!

@nnnnoel Thank you very much for providing this diff! Looks very clear! 😃

However, I've noticed that this PR not only changed format, but also changed the generated "enum" -> "const enum".

From what I read on the link you provided, there are difference in behaviors between them — "Const enums can only use constant enum expressions and unlike regular enums they are completely removed during compilation."

I wonder if some of our clients might be relying on the old format and can potentially be broken by this change?

@nnnnoel
Copy link
Copy Markdown
Contributor Author

nnnnoel commented Sep 19, 2024

Thanks for read docs @sampajano .

It has breaking change when use enum as enum key, but the typescript enum's bi-directional bind spec is not valid in grpc protocol serializer spec.
so people used it like above would not have been able to use gRPC.

@sampajano
Copy link
Copy Markdown
Collaborator

sampajano commented Sep 20, 2024

@nnnnoel Ahh i see! Thanks for explaining! That makes sense!

But could you elaborate a bit further on what you mean by "typescript enum's bi-directional bind spec is not valid in grpc protocol serializer spec."? Could you give me a quick example maybe?

Sorry for the trouble but I want to understand a bit better before introducing a potentially breaking change 😃

@nnnnoel
Copy link
Copy Markdown
Contributor Author

nnnnoel commented Sep 20, 2024

@sampajano

Before

var key = RefreshTokenResponseStatus[RefreshTokenResponseStatus.REFRESH_TOKEN_SUCCESS]
//    ^ "REFRESH_TOKEN_SUCCESS"

After

var key = RefreshTokenResponseStatus[RefreshTokenResponseStatus.REFRESH_TOKEN_SUCCESS]
//    ^ TypeError

@sampajano
Copy link
Copy Markdown
Collaborator

sampajano commented Sep 21, 2024

@nnnnoel Got it!! Thanks SO much for the clarification! Makes perfect sense now!

Thank you again for your contribution and patience! 😃

sampajano
sampajano approved these changes Sep 21, 2024
View reviewed changes
Copy link
Copy Markdown
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 again!! 😊

@sampajano sampajano merged commit 7bd92c6 into grpc:master Sep 21, 2024
@SidStraw SidStraw mentioned this pull request Sep 10, 2025
Closed
@sampajano sampajano mentioned this pull request Sep 11, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sampajano sampajano sampajano approved these changes

Assignees

No one assigned

Labels

release notes: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nnnnoel @sampajano @grpc-kokoro