Skip to content

fix: format of typescript definition #1463

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 21, 2024
Merged

fix: format of typescript definition #1463

merged 1 commit into from
Sep 21, 2024

Conversation

nnnnoel
Copy link
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
Collaborator

@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
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
Collaborator

@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
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
Collaborator

@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
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
Collaborator

@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
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
3 checks passed
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