Considerations about extending functionalities and fixing conceptual issues

[slavik57]

Considerations about extending functionalities and fixing conceptual issues

[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