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

[js] enum-as-object cleanup wishlist #7165

Open
nadako opened this issue Jun 8, 2018 · 8 comments
Open

[js] enum-as-object cleanup wishlist #7165

nadako opened this issue Jun 8, 2018 · 8 comments
Assignees
Labels
enhancement platform-javascript Everything related to JS / JavaScript
Milestone

Comments

@nadako
Copy link
Member

nadako commented Jun 8, 2018

So I diffed the output with the new object-based enums and it looks great, however it increases code size in real-world cases quite a bit and I think we can do something about it:

enum E {
	A;
	B(v:Int);
}

generates

var E = { __ename__ : true, __constructs__ : ["A","B"] };
$hxEnums["E"] = E;
E.A = {_hx_index:0};
E.A.toString = $estr;
E.A.__enum__ = "E";
E.B = function(v) { var $x = {_hx_index:1,v:v,__enum__:"E"}; $x.toString = $estr; return $x; }
E.B.__params__ = ["v"];;

could be optimized to

var E = $hxEnums["E"] = { __ename__ : true, __constructs__ : ["A","B"] };
E.A = {_hx_index:0, toString: $estr, __enum__: "E"};
E.B = ($_ = function(v) { return {_hx_index:1,v:v,__enum__:"E",toString: $estr} }, $_.__params__ = ["v"], $_);

Basically we don't want to repat the type/ctor name too much because in real code it would be something like rambo_ResourceChangeReason.ClanwarAttackMaterialPurchase.


Another thing that makes me uncomfortable is the naming inconsistency here. We have _hx_index, but also __enum__ and $hxEnums. I feel like it would be better to use the $ident scheme for everything as this would also exclude the possibility of name clashes coming from weirdly-named identifiers in Haxe code.

So something like E.A = {$enum: "E", $index: 0, toString: $estr};. I think it would also look better when using the enum objects from plain JavaScript, as the $-fields look like special tag fields while non-$-fields contain data.


Thoughts?

@nadako nadako added enhancement platform-javascript Everything related to JS / JavaScript labels Jun 8, 2018
@nadako nadako added this to the Release 4.0 milestone Jun 8, 2018
@ncannasse
Copy link
Member

Yes $ prefix is a better escaping sequence for JS output since it's not a valid Haxe identifier

@nadako
Copy link
Member Author

nadako commented Jun 9, 2018

Another thing - since we have $hxEnums now, it makes sense to make Type.resolveEnum use it instead of also putting enums into $hxClasses.

@nadako
Copy link
Member Author

nadako commented Jun 10, 2018

It's better now:

var E = $hxEnums["E"] = { __ename__ : true, __constructs__ : ["A","B"] };
E.A = {_hx_index:0,__enum__:"E",toString:$estr};
E.B = ($_=function(v) { return {_hx_index:1,v:v,__enum__:"E",toString:$estr}; },$_.__params__ = ["v"],$_);

still need to rename the fields ($enum and $index?)

another idea is to merge the declarations into one object, so we don't repeat the type name at all:

var E = {
  __constructs__: ["A", "B"],
  A: {$index: 0, $enum: "E"},
  B: ($_=function(v) { return {$index: 1, $enum: "E", v:v} },$_.__params__=["v"],$_),
}

@ncannasse
Copy link
Member

$enum and $index is good for me. and yes the litteral declaration seems better

@Simn
Copy link
Member

Simn commented Dec 1, 2018

Do you still want to do something here for 4.0?

@nadako
Copy link
Member Author

nadako commented Dec 1, 2018

I wanted to unify impl fields to use $ instead of double underscores and _hx_ prefixes, but it's a bit annoying to implement because we want to access those fields from haxe code (Type.enumIndex etc). Not a priority for 4.0, I'd say.

@nadako
Copy link
Member Author

nadako commented May 24, 2020

I still think we should use $-prefixed fields, but it can't be done in a non-ugly way without #5105 or #9433 in place, because, as mentioned, we need to access them in the reflection API.

@nadako
Copy link
Member Author

nadako commented May 24, 2020

Also I don't like the fact that __enum__ is a string. IMO it should cycle-reference the enum definition object and the JSON round-trip argument seem weak to me (direct JSON.stringify on enums is not something anyone would want to do anyway).

So if there are no arguments against changing this, I'll do this at some point when I have time for it :)

@RealyUniqueName RealyUniqueName modified the milestones: Release 4.2, Backlog Dec 14, 2020
@Simn Simn removed this from the Backlog milestone Mar 24, 2023
@Simn Simn added this to the Later milestone Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement platform-javascript Everything related to JS / JavaScript
Projects
None yet
Development

No branches or pull requests

4 participants