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

Review TypeScript conversion #161

Closed
jbphet opened this issue Apr 14, 2022 · 3 comments
Closed

Review TypeScript conversion #161

jbphet opened this issue Apr 14, 2022 · 3 comments
Assignees

Comments

@jbphet
Copy link
Contributor

jbphet commented Apr 14, 2022

In today's developer/typescript meeting we decided to have set of reviews on new and converted TypeScript code in order to get feedback, cross pollinate techniques and ideas, and generally improve the quality of the code. I requested that tambo be reviewed, and @jonathanolson was assigned to do it.

I've set up a meeting for tomorrow (4/15/2022) with @jonathanolson to review some specific questions that I have and set some general direction for the review.

@jbphet
Copy link
Contributor Author

jbphet commented Apr 14, 2022

For reference, the issue under which the TS port was done is #160.

@jonathanolson
Copy link
Contributor

jonathanolson commented Apr 21, 2022

The typescript overall looks great! I feel like I only have nit-picky things where I've done things differently. Notably:

  • There are cases of @public, @private, @constructor, etc. that are unneeded (e.g. AmplitudeModulator, MultiSelectionSoundPlayerFactory)
  • There are cases of duplicated documentation and type info at field declaration and when they are set in constructors (e.g. AmplitudeModulator, ContinuousPropertySoundGenerator.remainingFadeTime, MultiClip.audioContextStateChangeListener). Presumably they should only be documented/typed/visibilitied at the field declaration site.
  • There are cases of missing return types in many places (.e.g base64SoundToByteArray, BinMapper.mapToBin)
  • There are cases of duplicated documentation and type info in the type declarations AND in the optionize call. soundManager.addSoundGenerator is showing types and documentation in its optionize - that should be moved (or only) in the type, so we're not duplicating? NoiseGenerator also.
  • Some things like soundManager.masterOutputLevel/reverbLevel, MultiClip.playAssociatedSound don't document parameter types
  • BinMapperOptions has a required option but is itself optional in the constructor of BinMapper? Expect to either be required in constructor, or optional in the type.
  • Would expect providedOptions/optionize in BinMapper constructor
  • Should SoundGeneratorInitializationOptions's categories be a string union (cheap enumeration) instead of allowing any strings?
  • Lots of fields are being made public as NumberProperty/BooleanProperty. This prevents swapping them with DerivedProperty/etc. in the future. Do we want to type them more generally, e.g. Property?
  • Some required providedOptions with an Options type with nothing required (MultiClip). Presumably use providedOptions?:
  • Cases like PitchedPopGenerator's local variables (oscillator/gainNode) where the type is documented as a comment could be changed to explicit type declarations, e.g. const oscillator: OscillatorNode =

EDIT: I (@jbphet) changed the list to checkboxes.

@jbphet
Copy link
Contributor Author

jbphet commented May 19, 2022

I've addressed all of the suggestions - thanks for the review @jonathanolson! Closing.

@jbphet jbphet closed this as completed May 19, 2022
jbphet added a commit that referenced this issue May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants