Closed KatieWoe closed 2 years ago
This is really easy to duplicate manually on the current master branch - just start the sim, go into any screen, and fire the cannon.
@samreid - In https://github.com/phetsims/tandem/commit/24fa184031e3001236fd0b666bc8a897a5a09d0d you added an assertion to PhetioGroup.getElement
. This change broke projectile-motion, which was assuming that this method acted like a native JavaScript array and returned undefined
. The code in projectile-motion can of course be modified to accommodate this change, but I thought I'd double check: Are you sure this is the behavior we want? It's not what the native Array
does. Is this what our ObservableArray
does (I took a quick look, but the current return type of ObservableArray.get
is any
, so it didn't help)?
The assertion was recommended by @zepumph in https://github.com/phetsims/tandem/issues/262#issuecomment-1122699349 and it sounded reasonable to me.
Since Array access returns type T and not T | undefined, that made sense to me. But I can also see the reasoning behind matching JS more closely. @zepumph or @jbphet can you please comment?
Here is the line of code breaking:
In my opinion. We should continue with how PhetioGroup is implemented. I think it is more of a code smell to rely on array[-1] to return undefined than it is to deviate from the javascript array. If we come to a case where it seems more reasonable to all undefined, then we can go from there, but this is (IMO) a small price to pay for a bit more type safety in the project.
The code was simple enough that I thought I'd fix the assertion eagerly. @jbphet can you please comment on this conversation, and let me know if you are alright with the commit above?
I think it is more of a code smell to rely on array[-1] to return undefined than it is to deviate from the javascript array.
I'm not sure I agree with this, and I have a hard time with arguments based on "code smell", since it seems rather subjective. I just tried some test code that used -1 as index for an ObservableArray
(see below), and it did not assert and returned undefined
. The code looked like this, and was added to WavesModel.ts
in greenhouse-effect, since it was convenient.
this.waveAtmosphereInteractions = createObservableArray( {
tandem: tandem.createTandem( 'waveAtmosphereInteractions' ),
phetioType: createObservableArray.ObservableArrayIO( WaveAtmosphereInteraction.WaveAtmosphereInteractionIO ),
phetioDocumentation: 'Interactions between IR waves coming from the ground and the atmosphere'
} );
// Temporary test code.
const myWaveInteraction = this.waveAtmosphereInteractions.get( -1 );
console.log( `myWaveInteraction = ${myWaveInteraction}` );
I also just tried the following on the JS console in Chrome:
var mySet = new Set();
undefined
mySet[-1]
undefined
myMap = new Map()
Map(0) {size: 0}
myMap.get( -1 )
undefined
So JS native arrays, sets, and maps all return undefined
when -1 is provided as an index, and our own ObservableArray
does the same. I would argue that it would be more consistent, and thus predictable for developers using PhetioGroup
, if it behaved the same way as other collections.
@jbphet's argument makes sense to me (but I don't feel ObservableArray is an independent vote since it extends the native array via the Proxy strategy) and in https://github.com/phetsims/tandem/issues/262#issue-1229996266 I said:
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
But later I was surprised to see that the type checker for array access is that arrays return type T
, not T|undefined
. You can see that from this experiment:
Note this is inconsistent with the typing for Map, which returns type T | undefined
:
TypeScript 4.1 added a compiler option to treat all array accesses as if they could return undefined
: https://stackoverflow.com/a/50647536/1009071, but that sounds like a lot of extra work to gain a little extra type safety. Upon reflection, I think the idea to get the type safety boost in PhetioGroup via an assertion (or even a throw Error
?) seems preferable. I guess I would keep this step out of ObservableArray if it still extends native array.
Also I'm happy to be outvoted and go with the team preference on this one.
I guess I would keep this step out of ObservableArray if it still extends native array.
Right! I forgot that our current implementation of this is an Actual array. I feel like Javascript is an incredibly type unsafe language, and one of its quirks (not features) to me is its grace in array accessing. ObservableArray aside, I think it would be a better trajectory for all our PhET array-like internals to try to improve upon this implementation with greater type safetey.
I also think that @jbphet and @samreid @zepumph's opinions are clear at this point, and that we should bring this to dev meeting to see what everyone thinks about it.
We discussed this at dev meeting.
T
and not T|undefined
. @samreid ran the type checker with noUncheckedIndexedAccess: true
and saw 1900 failures. We agree that it isn't worth going back to fix those usages.I'll proceed with that finding here and in https://github.com/phetsims/tandem/issues/262#issuecomment-1122699349.
In this case https://github.com/phetsims/projectile-motion/commit/94789487e60ccb43b57ddddab623291e0d376dc0 will stick around. Closing