phetsims / phet-core

Core utilities used by all PhET simulations.
MIT License
8 stars 6 forks source link

Is EnumerationValue.isEnumerationValue still needed? #98

Closed zepumph closed 2 years ago

zepumph commented 2 years ago

I think this may be obsolete since adding a bit more structure to EnumerationValue. I'll take a look.

From https://github.com/phetsims/chipper/issues/1106

zepumph commented 2 years ago

@samreid, I didn't see any type errors here, do you think we are good to close this issue?

samreid commented 2 years ago

We needed isEnumerationValue when EnumerationValue read:

class EnumerationValue {
  name?: string; // undefined until set by RichEnumeration

  toString() {
    return this.name;
  }
}

So I think we may still need it. But I tried deleting the body of EnumerationValue to try to reproduce the problem we saw before, and I couldn't recall the context.

zepumph commented 2 years ago

I thought that name, enumeration, and sealedCache would now be enough to structurally type this unique class. I'm glad you can't reproduce the original issue. Please reopen if you have any other thoughts.

samreid commented 2 years ago

name and enumeration are both optional, so objects without those will match the interface. The sealedCache is static so it doesn't appear in the EnumerationValue interface.

zepumph commented 2 years ago

Ahh, thanks. Yes that makes perfect sense. Reverted above. Closing.