three-ts-types icon indicating copy to clipboard operation
three-ts-types copied to clipboard

fix wrong typed `Euler.toArray()`, add `EulerOrder` for type safety

Open noname0310 opened this issue 3 years ago • 6 comments
trafficstars

Why

array?: number[] of the existing Euler.toArray() method is a typing that does not fit the implementation of the method.

This is the original d.ts code.

toArray(array?: number[], offset?: number): number[];

This is the implementation of three.js Euler.toArray()

toArray( array = [], offset = 0 ) {

	array[ offset ] = this._x;
	array[ offset + 1 ] = this._y;
	array[ offset + 2 ] = this._z;
	array[ offset + 3 ] = this._order; //`this._order` type is string so array should not be number[]

	return array; //return value is the same as the above.

}

What

The array parameter of Euler.toArray() was changed to array?: any[] and the return type was also matched.

toArray(array?: any[], offset?: number): any[];

In addition, the EulerOrder type was added for type safety of the order and Euler.fromArray() parameter type was changed more correctly

export type EulerOrder = 'XYZ' | 'YXZ' | 'ZXY' | 'ZYX' | 'YZX' | 'XZY';
fromArray(xyzo: [number, number, number, EulerOrder?, ...any[]]): Euler;

Checklist

  • [x] Added myself to contributors table
  • [x] Ready to be merged

noname0310 avatar Jan 06 '22 18:01 noname0310

I'm not sure changing the array to be any is correct... 🤔

joshuaellis avatar Jan 06 '22 19:01 joshuaellis

It is not, but neither is number[].

// https://github.com/mrdoob/three.js/blob/dev/src/math/Euler.js#L267
fromArray( array ) {

  this._x = array[ 0 ];
  this._y = array[ 1 ];
  this._z = array[ 2 ];
  if ( array[ 3 ] !== undefined ) this._order = array[ 3 ];

  this._onChangeCallback();

  return this;

}

toArray( array = [], offset = 0 ) {

  array[ offset ] = this._x;
  array[ offset + 1 ] = this._y;
  array[ offset + 2 ] = this._z;
  array[ offset + 3 ] = this._order;

  return array;

}

CodyJasonBennett avatar Jan 23 '22 11:01 CodyJasonBennett

Is this because order could be a string? Or.... it'd be helpful if we had a basic example to review how this is a problem

joshuaellis avatar Jan 23 '22 13:01 joshuaellis

Isn't the return type something like [number, number, number, string]? Or does the offset arg break that?

alexandernanberg avatar Mar 06 '22 15:03 alexandernanberg

Isn't the return type something like [number, number, number, string]? Or does the offset arg break that?

This code is not easy to typed because of offset.

noname0310 avatar Mar 25 '22 12:03 noname0310

I think realistically it has to be Array<number | string | undefined> in an ideal world it would have been [x:number, y:number, z:number, offset:string], but thats not possible.

Take this example:

toArray( array = [], offset = 0 ) {

  array[ offset ] = this._x;
  array[ offset + 1 ] = this._y;
  array[ offset + 2 ] = this._z;
  array[ offset + 3 ] = this._order;

  return array;

}

because you can define an offset we have no idea at what index each value type would be so the forth argument could be a number because you set the offset as 1, in addition to this if you passed an empty array but set the offset to 1 then index 0 would be undefined.

joshuaellis avatar Mar 26 '22 16:03 joshuaellis

I solved the conflict and modified the any[] to Array<number | string | undefined>

noname0310 avatar Sep 08 '22 05:09 noname0310