search

phetsims / tandem

Simulation-side code for PhET-iO
MIT License
0 stars 5 forks source link

PhetioGroup.getElement return type improvements #262

Closed samreid closed 2 years ago

samreid commented 2 years ago

Related to #254, @marlitas and I observed that PhetioGroup currently has:

  getElement( index: number ): T {
    return this._array[ index ];
  }

However, it should really have return type T | undefined since there is no guard on the index. Alternatively, we could throw an error if the index is out of bounds and keep the specific return type of T, but that would be inconsistent with how the language treats array indices/access. @zepumph what do you recommend?

We observed 10 or so occurrences that would need updating if we make this T | undefined.

samreid commented 2 years ago

Same decision should apply to the new method getLastElement.

zepumph commented 2 years ago

I think that we should assert that index is in range. And we could do a type assertion/coercion as well to make typescript happy.

samreid commented 2 years ago

I was surprised to see that array access already returns the array type (without undefined). So we don't need type assertion or coercion. Fixed and ready for review.

zepumph commented 2 years ago

More discussion in https://github.com/phetsims/projectile-motion/issues/277. We aren't going to change the typescript compiler option for all array accesses, but we are keeping the PhetioGroup assertion. Closing