Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PhetioGroup.getElement return type improvements #262

Closed
samreid opened this issue May 9, 2022 · 4 comments
Closed

PhetioGroup.getElement return type improvements #262

samreid opened this issue May 9, 2022 · 4 comments

Comments

@samreid
Copy link
Member

samreid commented May 9, 2022

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
Copy link
Member Author

samreid commented May 9, 2022

Same decision should apply to the new method getLastElement.

@zepumph
Copy link
Member

zepumph commented May 10, 2022

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
Copy link
Member Author

samreid commented May 11, 2022

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
Copy link
Member

zepumph commented May 26, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants