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

embind enums aren't compatible with jasmine equality checks #19387

Open
mitchellwills opened this issue May 17, 2023 · 3 comments
Open

embind enums aren't compatible with jasmine equality checks #19387

mitchellwills opened this issue May 17, 2023 · 3 comments
Labels

Comments

@mitchellwills
Copy link

Version of emscripten/emsdk: 3.1.35

All enum values of a given enum type generated by embind are considered equivalent by jasmine. Jasmine does object equality by doing a deep equals on most objects; however, this relies on being able to iterate over the object keys (using Object.keys). embind enum values are all objects with a single non-enumerable property with the underlying value. Since the value property isn't enumerable the object looks the same. For single values this is somewhat ok because the toBe matcher can be used instead; however, it becomes problematic when comparing the quality of objects/arrays using toEquals because the same logic is applied recursively.

For example

module.MyEnum.VALUE1 === module.MyEnum.VALUE2 // false
module.MyEnum.VALUE1 == module.MyEnum.VALUE2 // false
expect(module.MyEnum.VALUE1).toBe(module.MyEnum.VALUE2);  // assertion failes
expect(module.MyEnum.VALUE1).toEqual(module.MyEnum.VALUE2);  // assertion incorrectly passes
expect([module.MyEnum.VALUE1]).toEqual([module.MyEnum.VALUE2]);  // assertion incorrectly passes

The setup code simplifies to roughly something like the following:

  it('enum equality', () => {
    const MyEnum = {};

    var Value1 = Object.create(MyEnum.constructor.prototype, {
      value: {value: 1},  // not enumerable
    });
    var Value2 = Object.create(MyEnum.constructor.prototype, {
      value: {value: 2},  // not enumerable
    });

    expect(Value1 !== Value2).toBe(true);
    expect(Value1 != Value2).toBe(true);
    expect(Value1).toEqual(Value2);  // always passes
    expect([Value1]).toEqual([Value2]);  // always passes
  });

One option to fix this could be to simply make the value property enumerable, but not sure if that would be a breaking change.

@sbc100
Copy link
Collaborator

sbc100 commented May 17, 2023

@brendandahl

@brendandahl
Copy link
Collaborator

Do you know offhand if this is how TypeScript generates enums?

@mitchellwills
Copy link
Author

Something along the lines of the following (playground link). Basically a bidirectional map between keys and values.

"use strict";
var MyEnum;
(function (MyEnum) {
    MyEnum[MyEnum["Value1"] = 1] = "Value1";
    MyEnum[MyEnum["Value2"] = 2] = "Value2";
    MyEnum[MyEnum["Value3"] = 3] = "Value3";
})(MyEnum || (MyEnum = {}));

I was wondering why emscripten didn't emit a simply dictionary of names and values for the enum like the following. It would mean you loose types for the enum, but would grant more direct access to the numeric values. This could also be typed cleanly in typescript typings (basically just declared as a normal typescript enum.

const MyEnum = {
  Value1: 1,
  Value1: 2,
  Value1: 3,
};

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

No branches or pull requests

3 participants