search

phetsims / mean-share-and-balance

"Mean: Share and Balance" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
2 stars 1 forks source link

type for ApplesAnimationState #299

Closed jessegreenberg closed 6 days ago

jessegreenberg commented 2 weeks ago

Hey, I was curious about the type used for ApplesAnimationState.

export type ApplesAnimationState = [ 'split' ] | [ 'land' ];

I expected a string literal union, what is the benefit of using an array?

For #270.

marlitas commented 1 week ago

Okay it's all coming back to me. I noticed when I was setting this up, that the emitter was allowing me to pass an array of the strings without throwing an error:

image

I wasn't a fan of that, and wanted to be explicit that you can only pass through one parameter and it must be either 'land' or 'split'. I'm honestly not sure if it's entirely necessary, so I'm open to being convinced otherwise.

jessegreenberg commented 1 week ago

I see, thanks. I changed the pattern just a little based on examples I saw elsehwere in PhET code. I think it makes the type for ApplesAnimationState a little more clear. What do you think of this?

jessegreenberg commented 1 week ago

As a side note, I was curious why TinyEmitter was used? I usually only see that in very performance/memory critical places. Many examples in code use an Emitter, which also supports a runtime check for the arguments passed to emit. 

 const e1 = new Emitter<[ number ]>( {
    parameters: [ { valueType: 'number' } ]
  } );

To be honest, I don't know why PhET uses parameters when we have TypeScript. Just pointing it out in case you want to use it.

marlitas commented 1 week ago

https://github.com/phetsims/mean-share-and-balance/commit/902aa247668c1814d0f2935ef8fea0ab952ef094 is much nicer. Thank you.

Oh that's good to know. I used TinyEmitter because I thought we were supposed to use that unless we needed more of the weight from Emitter, but perhaps that validation is enough reason to do so. I'll go ahead and switch to Emitter. Thanks for investigating!

marlitas commented 6 days ago

I went ahead and used Emitter instead of TinyEmitter. I believe this issue can be closed now.