Considerations about extending functionalities and fixing conceptual issues

[t-ricci-enhancers]

Hi @slavik57 and everyone,

Nice work we've got here, me and my colleagues have been using the enum-values npm package in a couple of our work projects and we kinda liked it.

I'm currently elaborating and working on a code solution for a pull request that spans over a couple new functionalities and fixing some conceptual issues. I'll try to keep this short and simple, and if you want we can decide together over two possible approaches for the next version's release.

1.st Issue: getting the name(s) from a value

I've seen there is a conceptually flawed mapping between names and values: we have unique name keys and (possibly) non-unique values. So if I want to get the value for an unique name (relation 1-to-1) I'll surely get one value as currently implemented (ok, then). But we don't necessarily have the opposite, that is getting names from non-unique values.

I'll post an example here...

TypeScript file:

enum MyEnum {
    VALUE1 = 1,
    VALUE2 = "VALUE2",
    VALUE_ONE = 1,
    VALUE_TWO = "VALUE2",
    VALUE_UNIQUE = "VALUE_UNIQUE"
}

Compiled JavaScript output:

var MyEnum;
(function (MyEnum) {
    MyEnum[MyEnum["VALUE1"] = 1] = "VALUE1";
    MyEnum["VALUE2"] = "VALUE2";
    MyEnum[MyEnum["VALUE_ONE"] = 1] = "VALUE_ONE";
    MyEnum["VALUE_TWO"] = "VALUE2";
    MyEnum["VALUE_UNIQUE"] = "VALUE_UNIQUE";
})(MyEnum || (MyEnum = {}));

which in the end boils down to this JavaScript object:

{
    "1": "VALUE_ONE",
    "VALUE1": 1,
    "VALUE2": "VALUE2",
    "VALUE_ONE": 1,
    "VALUE_TWO": "VALUE2",
    "VALUE_UNIQUE": "VALUE_UNIQUE"
}

As you can see here we have two names that "map" to the same value (1) and two names that "map" to another identical value ("VALUE2")

First possibility:

We can release a minor version update (1.2.x) with the deprecated old methods (for compatibility with existing projects' code) and add two new methods: getNamesFromValue (note the "names" is plural, which indicates the return type is string[]) and another one that fixes the existing getNamesAndValues with a different name (I can't decide a good wording for it, right now). We can in a later major version release (1.y.x) remove the old methods and keep the new ones

Second possibility:

We could directly make a major version (1.y.x) with the renamed method getNamesFromValue and string[] return type (let's keep the implementation part separate for now, I'm still working on the code as we speak)

2.nd Proposal: name Map / values Map

This is a new feature request, that I'm trying to implement here: https://github.com/t-ricci-molecle/enum-values/blob/enum-map/src/enumValues.ts

This methods would return an ES6/ES-2015 Map which, for one method, can be repeatedly used (with possibly a time complexity of O(1) if supported by the browser and not polyfilled) to access the values from the given name, or to access the name (which as issue 1 explains should be a names array) from the given value.

The use case of these two methods (we can name them getNameMap or getNamesMap and getValueMap or getValuesMap) can be critical when you don't want to call too many times the old (or fixed) methods: getNameFromValue(), getNames(), getValuesandgetNamesAndValues. Basically we'll be able to easily convert an Enum to one of the two Maps for fast access. I'd see well this feature in the enum-values` package instead of a separate npm module

3.rd consistency: getValueFromName

This is less practical, but for consistency if we have a getNameFromValue (fixed getNamesFromValue) we should also have the opposite method getValueFromName(MyEnum, myValue) (which would simply be the same as MyEnum[myValue]

Conclusion

@slavik57 let me know what you think about the 3 changes, I can either make separate Pull Requests or a single big one if we agree to bump the major version (we can create a branch for a feature release called 1.2.x, so we can do further development, changes and testing before releasing it on npm)

[slavik57]

Thanks a lot for the time you've put into this and I'm glad that the package helps you guys!

1.st Issue: getting the name(s) from a value

I'm not so sure about it, because as you can see from the compiled javascript, the mapping from 1 as a property has only the VALUE_ONE name. I'm not so sure it is a proper use of an Enum and I'm pretty sure that in static languages like C# or Java it will be a compilation time error to assign the same value to different Enum keys. Supporting this will make it harder for 99..99% of cases where you do have only 1 value for each key. Now you would need to read it as an array, This is why I don't think we should implement it.

2.nd Proposal: name Map / values Map

It should be implemented in such a way that it works even on older javascript versions or fails in a proper way explaining with an error that you must have ES6 Map in order for it to work. The rest of the package shouldn't fail in the browser and in nodejs for those who don't have es6.

3.rd consistency: getValueFromName

I didn't implement it because it is already implemented but the TS enum simply by using SomeEnum.Key. I don't see a point of implementing it since probably no one will use it.

Conclusion

So I think you can implement the 2nd proposal, create a PR. Update the .travis.yml file to check it both works on older and newer node versions. And again, thanks for your time and effort!

[Zorgatone]

Hi I'm replying from my personal account.

1.st

@slavik57 actually the compiled code results in a JS object with both names VALUE1 and VALUE_ONE. Same with VALUE2 and VALUE_TWO.

2nd

Of course, I intend to use core-js to use Map when available or use a polyfill (without polluting globals) when it's not available

3rd

It could just be there as a simple return MyEnum[myName] for reference. Doesn't have to be used necessarily but sometimes EnumValues.getValueFromName(MyEnum, myName) might better explain intent. Anyway I could keep this out for now

conclusion

Ok I'll try that

[slavik57]

1st

That is true, but only one 1 pointing to VALUE_ONE and nothing pointing to VALUE1. That's why I don't think we should support it either.

[Zorgatone]

{
    "1": "VALUE_ONE",
    "VALUE1": 1,
    "VALUE2": "VALUE2",
    "VALUE_ONE": 1,
    "VALUE_TWO": "VALUE2",
    "VALUE_UNIQUE": "VALUE_UNIQUE"
}

That's true for number values but for string values you don't get an object property in order to not confuse it with a name. So you'll still have to first look at all the values, and for each repeating value you'll have a different name. How is it defined for string values which name you should return? IMHO it isn't optimal to arbitrarily decide which name to return, and we should at least have a method which returns all of them.

return all.length == 1 ? all[0].name : null; PS: I wasn't sure about it and double-checked the code, I see you're checking filtered values' length and returning null in the case it's not one. Was that intentionally designed to return null both when you don't find a given value AND when you're finding duplicate values?

I was thinking in our work's codebase we had some instances of this example use case, now that I think about it we wouldn't be able to get the name from a value if we try to:

export enum MyExampleColors {
  DEFAULT = '#0000ff',
  BLUE = '#0000ff',
  RED = '#ff0000',
  GREEN = '#00ff00',
  GRAY = '#eeeeee',
  GREY = '#eeeeee'
}

I've actually seen multiple times a similar pattern in other colleagues' and my own's code, so for example I might want to do something like:

const myColor: MyExampleColors  = getColor() || MyExampleColors.DEFAULT;
function showColorName(color) {
  // Note that getNamesFromValue should always return an array (maybe empty)
  const names: string[] = EnumValues.getNamesFromValue(color).filter(c => c !== 'DEFAULT');
  return names.length > 0 ? names[0].toLowerCase() : 'unknown color';
}
console.log(showColorName(myColor));

[slavik57]

PS: I wasn't sure about it and double-checked the code, I see you're checking filtered values' length and returning null in the case it's not one. Was that intentionally designed to return null both when you don't find a given value AND when you're finding duplicate values?

It was to check for when I do not find anything.

Regarding the color example, I can now see your point more clearly. Maybe adding a method getNamesFromValue while leaving the getNameFromValue as is would work for backward compatibility.

[Zorgatone]

Ok, sounds fair