zmxv / react-native-sound

React Native module for playing sound clips
MIT License
2.78k stars 748 forks source link

v1.0.0 Typescript #721

Open antoine-pous opened 3 years ago

antoine-pous commented 3 years ago

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:

Known issues

Remaining changes:

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 commented 3 years ago

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:

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?

RomualdPercereau commented 3 years ago

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()
antoine-pous commented 3 years ago

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.

RomualdPercereau commented 3 years ago

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 commented 3 years ago

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 
antoine-pous commented 3 years ago

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

RomualdPercereau commented 3 years ago

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

antoine-pous commented 3 years ago

Yup, in few days

RomualdPercereau commented 3 years ago

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

antoine-pous commented 3 years ago

Not yet unfortunately :(

pe-johndpope commented 2 years ago

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

tahir-jamil commented 10 months ago

any updates