Make hashmap keys required unless the key is an enum

[gustavo-shigueo]

Goal

Remove optional key from hashmaps unless the key type is an enum
Closes #442
Closes #374

Changes

Added new const to TS trait that determines whether or not a type should have a ? when used as a hashmap key

Checklist

• I have followed the steps listed in the Contributing guide.
• If necessary, I have added documentation related to the changes made.
• I have added or updated the tests related to the changes made.

[NyxCode]

Thanks for opening the PR!
I don't believe { [k in K]: V } is the correct binding for non-enum keys. That would require all values of K to be present in the map, no?
Wouldn't { [k: K]: V } as a default, with { [k in K]?: V } for enum keys, be the better choice here?

For context, here's the history of our bindings for hashmaps:

• We started with Record<K, V>
• We replaced that with { [k: K]: V } to allow for recursive types (Replace Record<K, V> with { [key: K]: V } #277)
• We replaced that with { [k in K]?: V } to allow for enum keys (Change HashMap to export mapped types #339)

[gustavo-shigueo]

in vs : doesn't make a difference for string or number which are the most likely uses of HashMap, and this implementation should catch all uses where K is a union of specific values (i.e. an enum), so it shouldn't make a difference:

That said, I do like key: K better and it's a pretty easy change to make

[NyxCode]

I thought about this more deeply in the meantime:

• I think we should go ahead with this.
• We should instruct users to use noUncheckedIndexedAccess if they want to keep their indexing checked for missing keys, which we forced onto them in Question to recent change Records<K, V> to { [key: K]: V } #338.
• I apologize for me previous criticism. I think we should stick with { [k in K]: V }. As you said, it should be equivalent.
• For enums, we have to keep using { [k in K]?: V }.
  • for being able to create empty hashmaps
  • without it, indexing always gives a value even with noUncheckedIndexedAccess
• I am very concerned about the churn we create for our users by making these changes too often.
  • Maybe we should make it configurable through environment variables, or at least offer a flag to keep the current behavior.

[timephy]

Maybe we should make it configurable through environment variables

I propose configuration like this to be done with feature flags 🙏🏽

[NyxCode]

Maybe we should make it configurable through environment variables

I propose configuration like this to be done with feature flags 🙏🏽

Feature flags are convenient, but not the right tool for this, since they need to be additive.
Ideally, we'd have some way to configure this for the current crate only. But in the meantime, I think an environment variable to make migration to a new version easier is the way to go.

[gustavo-shigueo]

Hey @NyxCode, what would you want this env variable to look like?

By that I mean, do you want a bool that if set to true reverts back to the previous behavior? The other way around? Or something else entirely?

Also, Merry Christmas! Hope you're doing great! Cheers