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

v1.0.0 Typescript #721

Open
antoine-pous opened this issue Apr 22, 2021 · 12 comments
Open

v1.0.0 Typescript #721

antoine-pous opened this issue Apr 22, 2021 · 12 comments
Assignees

Comments

@antoine-pous
Copy link
Collaborator

antoine-pous commented Apr 22, 2021

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 Android - Set Volume Button Controls On Android Audio Stream #693 fix: xcode 12 compatibility #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
Copy link
Collaborator Author

antoine-pous commented Apr 22, 2021

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?

@RomualdPercereau RomualdPercereau pinned this issue Apr 22, 2021
@RomualdPercereau RomualdPercereau linked a pull request Apr 22, 2021 that will close this issue
@RomualdPercereau RomualdPercereau removed a link to a pull request Apr 23, 2021
@RomualdPercereau
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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

@RomualdPercereau
Copy link
Collaborator

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

@antoine-pous
Copy link
Collaborator Author

Yup, in few days

@RomualdPercereau
Copy link
Collaborator

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

@antoine-pous
Copy link
Collaborator Author

Not yet unfortunately :(

@pe-johndpope
Copy link

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

@tahir-jamil
Copy link

any updates

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

4 participants