Skip to content
This repository has been archived by the owner on Mar 23, 2020. It is now read-only.

Synth Config Options includes unused properties #15

Closed
jcapinc opened this issue Sep 3, 2019 · 2 comments
Closed

Synth Config Options includes unused properties #15

jcapinc opened this issue Sep 3, 2019 · 2 comments

Comments

@jcapinc
Copy link

jcapinc commented Sep 3, 2019

interface SynthOptions {
    oscillator?: OscillatorOptions<OscillatorType>;
    envelope?: ADSREnvelopeOptions;
  }

The SynthOptions includes the Oscillator Options type in its entirety:

interface OscillatorOptions<T = BasicOscillatorType> {
    type?: T;
    modulationType?: T;
    modulationIndex?: number;
    harmonicity?: number;
  }

According the The documentation for the Synth options, modulationType, modulationIndex, and harmonicity are not used on the synth at all, which makes sense.

Instead of including the entire OscillatorOptions type or breaking DRY principles by defining it over again, I propose using the "Pick" type tool to pull only the type type out of the OscillatorOptions list like so:

interface SynthOptions {
    oscillator?: Pick<OscillatorOptions<OscillatorType>, 'type'>;
    envelope?: ADSREnvelopeOptions;
  }

This would communicate to users of the Synth instrument that they only relevant oscillator configuration is the type, and they would not get lost wondering what the modulator and harmonicity are doing there (like I may have done for like an hour).

If this solution is acceptable I will make the change and submit a PR. LMK

@tambien
Copy link
Contributor

tambien commented Sep 4, 2019

Thanks for the issue @jcapinc , this is pretty comparable to how it's typed in the new typescript branch which is where i'm putting my effort instead of updating these type definitions. Feel free to submit a PR if that's helpful for you. Once the main repo is done with the typescript conversion, i will archive this repo.

Contributions on the typescript branch of Tone.js would also be really helpful!

@jcapinc
Copy link
Author

jcapinc commented Sep 4, 2019

ah! ok I did not understand that the typescripting effort had moved, thanks for the update I will go look at the typescript branch

@jcapinc jcapinc closed this as completed Sep 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants