react-native-sound icon indicating copy to clipboard operation
react-native-sound copied to clipboard

v1.0.0 Typescript

Open antoine-pous opened this issue 4 years ago • 12 comments

This is a work in progress, this issue is related to PR #713 and still need some fixes and requires a lot of tests. Any advice are welcome, the debate will continue here to give a better view about the changes which are applied to v1.0.0 branch.

What's new:

  • Moved from JS to Typescript (ES6)
  • Removed legacy JS lib and types definitions
  • All functions which call the Java bridge use Promises
  • Added an opt-out option rejectOnUnsupportedFeature {Boolean} which ensure that using an unsupported feature will be rejected. If disabled (default behavior) the function will be resolved
  • Added deterministic state functions:
    • enable / disable
    • setSpeakerphoneOn / setSpeakerphoneOff
    • enableInSilenceMode / disableInSilenceMode
    • setActive / setInactive
  • Added SoundBasePath type which can be 'MAIN_BUNDLE' | 'DOCUMENT' | 'LIBRARY' | 'CACHES' | string
  • Added SoundOptions interface with rejectOnUnsupportedFeature?: boolean and enableSMTCIntegration?: boolean
  • Implemented PR #693 #704
  • Now RNSoundModule.createMediaPlayer will execute the callback error with more explicit messages. Resource not found covered too much errors, this update will give a better view of what's happening exactly. Resource not found remain as last error when the function is not able to determine which action is requested, maybe should we put a better error message. Error payload have also a new key named resource which provide the full path of the fetched file to enhance debugging.

Known issues

  • when IsAndroid is true predefined directories are undefined using React Native with Expo

Remaining changes:

  • Updating the documentation
  • Updating the changelog
  • Unit tests & CI
  • Implement PR which are awaiting for approval

This is clearly a braking change and i don't see any reason to continue to support legacy stuff when most of supported versions of RN, by this lib, are not supported anymore. If people are not able to upgrade their dependencies and follow the new standards it's sad but IMO a library must remain updated, they can still use legacy versions of the library if needed.

Feel free to suggest any changes

To test it in your projects:

Using Yarn: yarn add zmxv/react-native-sound#1.0.0 Using NPM: npm install git://github.com/zmxv/react-native-sound.git#1.0.0 --save

Usage example

const sound: Sound = new Sound('sound.mp3', 'MAIN_BUNDLE', {rejectOnUnsupportedFeature: true})
sound.isLoaded().then(() => {
  sound.play()
})

antoine-pous avatar Apr 22 '21 08:04 antoine-pous

Yes, could be a way to have a dedicated namespace for these functions. It could give this kind of implementation. I don't know if Sound should be default in that case? @antoine-pous

import Sound, { Track } from "react-native-sound"

const track: Track = new Track("test.mp3", "MAIN_BUNDLE")
track.isLoaded().then(() => {
  track.play()
})

Sound.enableInSilenceMode()

@RomualdPercereau IMO we should move to a smart mechanism which control a bit more what's happening and offer more features.

For the current stage I think wrapping tracks into a private Track class and exposing Sound class would be useful.

Sound will provide a set of methods to manages Tracks using an internal Map of Track instances. Those methods can be:

  • Sound.addTrack(key: string, filePath: string, option?: TrackOption)
  • Sound.addTracks([{key: string, filePath: string, option?: TrackOption}], option?: TrackOption)
  • Sound.rmTrack(key: string)
  • Sound.playTrack(key: string)
  • Sound.pauseTrack(key: string)
  • Sound.stopTrack(key: string)
  • Sound.clearTracks() // to remove all tracks

Then the private Track class will be only to perform actions without direct interaction, which would avoid a lot of mistake, the Sound class will perform all required controls which will simplify this lib usage.

And then later, once stable and well tested, it will be simpler to add some other features.

PS: can you pin this issue please?

antoine-pous avatar Apr 22 '21 08:04 antoine-pous

Hello @antoine-pous

I don't know exactly how people are using react-native-sound. But not exposing the Track class and move to a string based api is a quite big change. (Other other opinions are welcome!)

It wouldn't be possible anymore to use it like this:

sound.js

export default {
  soundA: new Sound(require("./sound_a.mp3")),
  soundB: new Sound(require("./sound_b.mp3")),
}

anywhere

import Sounds from "./assets/sounds"
Sounds.sound2.play()

RomualdPercereau avatar Apr 23 '21 06:04 RomualdPercereau

This version is a breaking change, so in fact it will be hard to use it the old way 😅

The main goal is to avoid this kind of import and having a simplified way to use it in any place of your app.

You can still expose the Track class to use it from the old way, but the developer will lose the wrapper advantage and will have to do all the controls by himself.

And Sound should be renamed Player, I think it's a best name for the wrapper.

antoine-pous avatar Apr 23 '21 07:04 antoine-pous

Sure! I'm just not so sure that the new usage of Track is really a simplification since its require to manage a list of sounds & strings instead of only sounds object 🙂

Sounds.unexistingSound.play() // -> Typescript error before running
Player.play("unexistingSound") // -> Error on runtime

RomualdPercereau avatar Apr 23 '21 08:04 RomualdPercereau

I guess it's called Sound because the lib is called react-native-sound. The usage is to call the import something similar to the name of the lib, for example:

react-native-track-player -> TrackPlayer react-native-iap -> RNIap react-native-linear-gradient -> LinearGradient

Wouldn't be quite confusing to ?

import Player from react-native-sound 

RomualdPercereau avatar Apr 23 '21 08:04 RomualdPercereau

I understand, it can be confusing yeah. Then Track can remain Sound and Player an optional and enhanced way to manage a library.

antoine-pous avatar Apr 23 '21 08:04 antoine-pous

Looks great! Would you have time to do a PR?

RomualdPercereau avatar Apr 23 '21 08:04 RomualdPercereau

Yup, in few days

antoine-pous avatar Apr 24 '21 15:04 antoine-pous

Hello @antoine-pous, Did you find some time to progress on this? :)

RomualdPercereau avatar May 25 '21 08:05 RomualdPercereau

Not yet unfortunately :(

antoine-pous avatar May 31 '21 14:05 antoine-pous

had to give up on this library - maybe this PR would resolve. but for now users won't be getting sound fx.

pe-johndpope avatar Mar 22 '22 05:03 pe-johndpope

any updates

tahir-jamil avatar Aug 23 '23 09:08 tahir-jamil