-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[embind] Add a way to register enum values as number or string #25257
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
Conversation
501b164 to
fad3ab8
Compare
0c66d1a to
b705c2e
Compare
b705c2e to
6b09273
Compare
|
I splitted most of the code between the two, but I kept a common class in the TS types generation, as they are very close to one another |
6bfd0ba to
92d9adb
Compare
92d9adb to
bdb0eb8
Compare
|
Sorry for the delay here. I got some feedback from a few different projects that also want enum to behave differently and not have a On another topic, one downside I see to not using TS enums is you can compare different enums and it will NOT be an error. e.g. export type Dog = 'a'|'b';
export type Cat = 'a'|'b';
interface EmbindModule {
Dog: {a: 'a', b: 'b'};
Cat: {a: 'a', b: 'b'};
};
let module = {} as EmbindModule;
let myDog : Dog = module.Dog.a;
if (myDog == module.Cat.a) { // <--- this is not a compiler error
}vs declare enum Dog {
a = 'a',
b = 'b',
}
declare enum Cat {
a = 'a',
b = 'b',
}
let myDog : Dog = Dog.a;
if (myDog == Cat.a) { < --- compiler error
} |
Why I'm not using TS enums
I didn't find a clean way to bind the enum to a real TS enum. If I understand well, you suggest doing: // module.d.ts
declare enum Animal {
Dog = 1;
Cat = 2;
}But since this enum is only declared in a The only way to then make this enum declaration usable, is to add this at the root of // module.js, outside the module code
const Animal = {
Dog: 1,
Cat: 2,
};
// Add reverse mapping like real TS enums
Animal[1] = 'Dog';
Animal[2] = 'Cat';
export Animal;This means that:
I didn't like the idea of manually reimplementing what TS usually does under the hood. And even if I wanted to, I didn't know where to do this implementation. Thus, I went for a plain TS type. I also think that TS types have the benefit over TS enums of being easier to use and to overload. I prefer using them over TS enums in general (but this is a personal preference. TS enums are a bit clunky imo) I don't think the comparison problem you raised is that much of an issue. In the end, Why I'm using strings
We saw above that I couldn't easily implement real TS enums, so I went for: // module.d.ts
export type Animal = 'Dog' | 'Cat';
interface EmbindModule {
Animal: { Dog: 'Dog', Cat: 'Cat' },
}Would you suggest replacing it with the following implementation ? I think it's a bit strange to declare a union type of ints like this.. // module.d.ts
export type Animal = 1 | 2;
interface EmbindModule {
Animal: { Dog: 1, Cat: 2 },
}Also this is less close to real TS enums.
const enum Animal { Dog: "Dog", Cat, "Cat" };
console.log(Object.values(Animal)); // ["Dog", "Cat"]
const enum Animal { Dog: 1, Cat, 2 };
console.log(Object.values(Animal)); // ["Dog", "Cat", 1, 2]See https://www.typescriptlang.org/docs/handbook/enums.html for details Since my implementation only used strings, it was closer to a real TS string enum behaviour. Finally, using plain strings allows me to use the enum values without even needing the module: const a: Animal = "Dog"; // << doesn't need the module
// vs
const a: Animal = myModule.Animal.Dog: // << needs an instanciated moduleWith number I would need to do: const a: Animal = 1; // << What is 1 ? Dog, Cat ?
// vs
const a: Animal = myModule.Animal.Dog; // << needs an instanciated module to be readablePossible solutionIf people need the int values, I think we can add a parameter in the enum binding (and merge all implementations inside Int enums:enum_<Animal>("Animal", enum_value_type::integer)
.value("Dog", Animal::Dog)
.value("Cat", Animal::Cat)emits: // module.d.ts
export type Animal = 1 | 2;
interface EmbindModule {
Animal: { Dog: 1, Cat: 2 },
}String enums:enum_<Animal>("Animal", enum_value_type::string)
.value("Dog", Animal::Dog)
.value("Cat", Animal::Cat)emits: // module.d.ts
export type Animal = "Dog" | "Cat";
interface EmbindModule {
Animal: { Dog: "Dog", Cat: "Cat" },
}Legacy enums (default):enum_<Animal>("Animal", enum_value_type::legacy)
.value("Dog", Animal::Dog)
.value("Cat", Animal::Cat)emits: // module.d.ts
export type AnimalValue<T extends number> {
value: T
}
export type Animal = AnimalValue<1> | AnimalValue<2>;
interface EmbindModule {
Animal: { Dog: Animal<1>, Cat: Animal<2> };
}This handles the various cases. It's not as close to TS enums, but again I don't think it's that much of a problem. |
|
Hi :) Any new on this @brendandahl ? Do you want me to develop the change I suggested above ? |
|
I also can't see a way to make the TS enums work well in a definition file, so the suggested change sounds good. For
FWIW, that misses the point of strong type safety and why the
I'd also suggest against doing this. Another value of enums is you can change the underlying values and not have to update your code every place that you hardcoded the value. |
09683fa to
ab20b5f
Compare
brendandahl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, just a few things to address.
8f7989d to
2c2feca
Compare
|
Sorry I previously wasn't done with my changes but I pushed early that's why there were some obvious flaws ^^" I implemented the changes you requested and squashed all the commits together for a cleaner history. (Nothing changed but the tests since last review) |
|
It's still not working. I'll spend more time on this later this week. I'll re-request review when ready |
|
Will you have time to revisit this? |
|
Sorry I didn't have time recently. I'll work on this now and come back today with fixes |
dbae6be to
90e28ca
Compare
|
The bugs should be fixed. I also enhanced the docs to make it clearer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a new enum_value_type parameter to embind's enum_ registration function, allowing C++ enums to be represented in JavaScript as objects (default), plain numbers, or plain strings. This addresses multiple user requests for simpler enum handling.
Key changes:
- Adds
enum_value_typeenum class with three options:object(default),number, andstring - Updates enum registration and TypeScript generation to support all three representation types
- Adds comprehensive tests demonstrating usage of all three enum types
Reviewed changes
Copilot reviewed 13 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| system/include/emscripten/wire.h | Defines the new enum_value_type enum class with three options |
| system/include/emscripten/bind.h | Updates enum_ constructor to accept optional enum_value_type parameter |
| src/lib/libsigs.js | Updates function signature for _embind_register_enum to include new parameter |
| src/lib/libembind_shared.js | Adds helper function to convert raw enum value type to string representation |
| src/lib/libembind.js | Implements runtime behavior for all three enum types with registration and value handling |
| src/lib/libembind_gen.js | Updates TypeScript generation logic to emit appropriate type definitions for each enum type |
| test/embind/embind_test.cpp | Adds test enums and functions for number and string enum types |
| test/embind/embind.test.js | Adds comprehensive test cases for number and string enum behaviors |
| test/other/embind_tsgen.cpp | Refactors test to use three different enum types (FirstEnum, SecondEnum, ThirdEnum) |
| test/other/embind_tsgen_main.ts | Updates test code to use refactored enum names |
| test/other/embind_tsgen*.d.ts | Updates TypeScript definitions to reflect new enum types and their representations |
| test/test_other.py | Adds comment noting test coverage for both default and string enums |
| site/source/docs/porting/connecting_cpp_and_javascript/embind.rst | Adds comprehensive documentation with examples for all three enum types |
| site/source/docs/api_reference/bind.h.rst | Updates API documentation for enum_ constructor with new parameter details |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
90e28ca to
ef6f612
Compare
Head branch was pushed to by a user without write access
4833f05 to
cefaa7c
Compare
|
I edited last commit because some tests were failing after merge with main |
|
Thanks! |
Following #25257 The docs are misformatted @brendandahl
Following #25257 The main change is that there was an inconsistency in the enum values between the three examples
This PR adds a param to
enum_to be able to use enums values as plain string or plain numbers in javascriptInt enums:
emits:
String enums:
emits:
Object enums (default):
emits:
This doesn't conflict with current implementation of enums, as the parameter default value is the one keeping the same behaviour
Fixes: #24324, #19387, #18585