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

suggestion: Change wire format for enums to integer values #12

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

arilotter
Copy link

Foreword

RIDL lets you specify integer values for enums.

enum Kind: uint32
  - USER  = 0
  - ADMIN = 1

On the Golang side, the generated code has access to enum values. e.g.

type Kind uint32

const (
	Kind_USER      Kind = 0
	Kind_ADMIN     Kind = 1
)

var Kind_name = map[uint16]string{
	0: "USER",
	1: "ADMIN",
}

var Kind_value = map[string]uint16{
	"USER":            0,
	"ADMIN":           1,
}

Problem

On the TypeScript side, there's no access to those values. e.g

enum Kind {
	USER = 'USER',
	ADMIN = 'ADMIN',
}

This means it's impossible to access those enum kind constants defined in RIDL from any TS code.

Proposed Solution

I propose that we change the generated TS enum values to include the real enum values.

enum Kind {
	USER = 0,
	ADMIN = 1,
}

That's what this draft PR shows.

To make this work, the wire format for enum values in WebRPC must also be changed. Since you can't know if a value is an enum value in TypeScript at runtime, we can't decide to keep sending the strings in the WebRPC client code.
I therefore propose that all WebRPC generated bindings change from transmitting enums as strings ("USER") to transmitting them as their enum values (1).
This will also have a side-effect of saving bytes on the wire, saving memory on the client side since there's less strings, and saving memory & CPU on the server side, since you don't have to do a reverse string lookup to find the real value again.

Required work

Change the code for reading/writing enums in TypeScript, Golang, JavaScript, and the generated Swagger/OpenAPI bindings to read/write raw enum integer values instead of strings.

@pkieltyka
Copy link
Member

what if we generated an additional enum type for TS ...?

We keep,

enum Kind {
	USER = 'USER',
	ADMIN = 'ADMIN',
}

and additionally we add...

enum KindValue {
	USER = 0,
	ADMIN = 1,
}

then, we keep it how it is, but we can still have the value of the enum too.. the reason it, this change would be a big breaking change.. a bit too much

@arilotter
Copy link
Author

Deal. PR ready for review.

@arilotter arilotter marked this pull request as ready for review October 27, 2023 20:45
@VojtechVitek
Copy link
Contributor

I'm not sure if the second ENUM is helpful, 'cause we're only sending and receiving strings over the wire in JSON. I don't think we can map between two different ENUMs, can we?

I think we'd rather need a mapping from string to numeric value, ie.:

enum Kind {
	USER = 'USER',
	ADMIN = 'ADMIN',
}

const KindValue = {
	[Kind.USER]: 0,
	[Kind.ADMIN]: 1,
} as const;

// Usage:
const incomingKind = Kind.ADMIN
const value = KindValue[incomingKind]

Thoughts?

@VojtechVitek
Copy link
Contributor

My team is also proposing to start using union types instead of ENUMs. They're easier to work with and have more advantages: https://www.typescriptcourse.com/string-literal-unions-over-enums.

type Kind = 'USER' | 'ADMIN'

const KindValue = {
	'USER': 0,
	'ADMIN': 1,
} as const;

What do you guys think about this?

Perhaps we could make this optional via webrpc-gen -target=typescript -enums=unions template option.

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

Successfully merging this pull request may close these issues.

None yet

3 participants