phetsims / bending-light

"Bending Light" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/bending-light
GNU General Public License v3.0
8 stars 8 forks source link

AngleNode `getRay` has incorrect return type #408

Closed pixelzoom closed 1 year ago

pixelzoom commented 2 years ago

Discoverd while working on

In AngleNode.ts:

/**
     * Select the ray of the given type 'incident' | 'reflected', or null if there isn't one of that type
     * @param type
     * @returns {LightRay}
     */
    const getRay = ( type: RayTypeEnum | null ) => {
      let selected = null;
      for ( let i = 0; i < rays.length; i++ ) {
        const ray = rays[ i ];
        if ( ray.rayType === type ) {
          assert && assert( selected === null, 'multiple rays of the same type' );
          selected = ray;
        }
      }
      if ( selected === null ) {
        return MOCK_ZERO_RAY;
      }
      return selected;
    };

@returns {LightRay} is incorrect, MOCK_ZERO_RAY is not a LightRay instance. It's a duck type:

const MOCK_ZERO_RAY = {
  getAngle: () => 0,
  powerFraction: 0
};

So the @returns cannot be properly replaced with a return type.

Not sure how you'd like to address this, so assigning to @samreid.

zepumph commented 2 years ago

Over in https://github.com/phetsims/chipper/issues/1224 I deleted that @returns to enabled a bad-typescript-text rule. Wouldn't we still want to use typescript in this case?

Type GetRay = ( x: RayTypeEnum | null ) => LightRayType;
const getRay: GetRay = . . . . . 
pixelzoom commented 2 years ago

No. getRay returns a LightRay in one case, a partial implementation of LightRay (duck type) in another.

This seems to work (uncommitted):

type LightRayDuckType = {
  getAngle: () => number;
  powerFraction: number;
};

const MOCK_ZERO_RAY:  LightRayDuckType  = {
  getAngle: () => 0,
  powerFraction: 0
};

const getRay = ( type: RayTypeEnum | null ):  LightRayDuckType => {
  ...
};
zepumph commented 2 years ago

Sounds good. However you want to type it. I wasn't trying to recommend the typescript code. I was trying to mention that having @returns in this case will be a lint error, and we should use typescript to accomplish this.

samreid commented 1 year ago

I added a type alias and specified it as the returned type. Closing.