hirbod / react-native-volume-manager

React Native module which adds the ability to change the system volume on iOS and Android, listen to volume changes and supress the native volume UI to build your own volume slider or UX. It can listen to iOS mute switch and ringer mode changes on Android (and let you set the ringer mode)
MIT License
216 stars 14 forks source link

Why is getVolume typed as number | VolumeResult? #15

Closed puckey closed 10 months ago

puckey commented 10 months ago
/**
 * Get the current device volume.
 * @param {AndroidVolumeTypes} type - The type of volume you want to retrieve. Defaults to 'music' (Android only, no-op on iOS).
 * @returns {Promise<VolumeResult>} - Returns a promise that resolves to an object with the volume value.
 *
 * @example
 * ```ts
 * const { volume } = await VolumeManager.getVolume('music'); // type is no-op on iOS
 * ```
 */
export async function getVolume(
  type: AndroidVolumeTypes = 'music'
): Promise<VolumeResult | number> {
  return await VolumeManagerNativeModule.getVolume(type);
}

With the return type of getVolume() being Promise<VolumeResult | number>, wouldn't the example code fail: const { volume } = await VolumeManager.getVolume('music');

Perhaps the number type may be removed as a return type?

hirbod commented 10 months ago

Thanks for pointing this out. You're correct that the current typing of the getVolume() function can be confusing. If the function returns a number, destructuring volume as in the example would indeed fail.

There are two options moving forward:

  1. I could modify the function to always return a VolumeResult. This means, in some cases, you would receive an object like { volume: number }, where volume represents the type of volume you requested.

  2. Alternatively, it might be safer to check the type of the returned value before trying to destructure it, like so:

const result = await VolumeManager.getVolume('music');
if (typeof result === 'number') {
  console.log('The volume is ' + result);
} else {
  console.log('The music volume is ' + result.music);
}

I'd appreciate your thoughts on which of these approaches you'd prefer. Alternatively, if you have other suggestions, I'd be open to considering them.

hirbod commented 10 months ago

Fixed in https://github.com/hirbod/react-native-volume-manager/releases/tag/v1.6.0 This release is a breaking change.

I've decided to always return VolumeResult (result.volume) and also include all android specific volumes as well.

puckey commented 10 months ago

@hirbod good choice - thanks for the new release!