E-Kuerschner / useAudioPlayer

React hooks for controlling audio on the web
MIT License
328 stars 36 forks source link

`AudioLoadOptions` shape does not correspond Howler constructor options and missing some of them #115

Closed PieceOfMeat closed 1 year ago

PieceOfMeat commented 1 year ago

Describe the bug AudioLoadOptions shape of load() function does not correspond Howler constructor options and missing some of them

To Reproduce Howler constructor expects options with names: mute, volume, rate. However, when I use load() function, AudioLoadOptions parameter suggests that these props should be named initialMute, initialVolume, initialRate, and useAudioPlayer code doesn't transform these names before passing them into Howl constructor.

Also, there are some howler options missing, I particularly miss preload option. I still can pass it into load() function, as it's just a new Howl(options) under the hood, but Typescript will look at me with disapproval.

Expected behavior I expect AudioLoadOptions shape to conform Howler constructor options, with option names mute, volume, rate, and with all possible options present.

Environment (please complete the following information): N/A

kovapatrik commented 1 year ago

I'm missing the sprite field! AFAIK it was supported in an older version of this package.

PieceOfMeat commented 1 year ago

@kovapatrik you can still use sprite in load() method and it will work just fine, though Typescript will complain - can use @ts-ignore as a workaround for now

kovapatrik commented 1 year ago

@kovapatrik you can still use sprite in load() method and it will work just fine, though Typescript will complain - can use @ts-ignore as a workaround for now

Yeah, true, I did tried this in the meantime, but for some reason pausing/resuming sprites looks buggy to me. It's definitely a howler.js issue, but I did a workaround by using the seek function and the example hook in the readme which tracks the live audio position.

E-Kuerschner commented 1 year ago

@PieceOfMeat, a quick note on preload: the way the hook is designed in v2 sort of removes the need for this option. Since the introduction of an explicit load function, you should be able to call this at your earliest convenience, even in different components. Does this satisfy the use case you are thinking of?

edit: i forgot about this detail, but in howler preload defaults to true. So if anything, this lib makes the process of loading audio a little more tedious by always forcing a call to load. I view this as a trade-off to allow devs to intentionally consider the moment they load the audio

E-Kuerschner commented 1 year ago

@PieceOfMeat @kovapatrik after thinking about sprites a bit, one issue I see is the lack of sound ids as they are supported in the base howler lib.

To be fair, my goal was never to mirror howler's api exactly and instead provide a set of utility hooks for controlling audio, but if features like sprites is something worth pursuing then i think we may need to approach the design of the hooks a bit differently.

For examples in howler:

const soundId = howl.play();
const secondSoundId = howl.play();

const isPlayingSound1 = howl.playing(soundId);
// pause first sound
howl.pause(soundId);
howl.play(soundId);
// similarly a sprite's name can be passed into play/pause/etc.

Currently, the lib doesn't support this type of behavior, but i think it could be updated to do so without breaking changes. Thoughts?

PieceOfMeat commented 1 year ago

@PieceOfMeat, a quick note on preload: the way the hook is designed in v2 sort of removes the need for this option. Since the introduction of an explicit load function, you should be able to call this at your earliest convenience, even in different components. Does this satisfy the use case you are thinking of?

Yes, I think so, thank you!

The issue with initialMute, initialVolume, initialRate names remains valid though, they're passed into howler's constructor as-is, and it expects to receive different names: mute, volume, and rate.

kovapatrik commented 1 year ago

@PieceOfMeat @kovapatrik after thinking about sprites a bit, one issue I see is the lack of sound ids as they are supported in the base howler lib.

To be fair, my goal was never to mirror howler's api exactly and instead provide a set of utility hooks for controlling audio, but if features like sprites is something worth pursuing then i think we may need to approach the design of the hooks a bit differently.

For examples in howler:

const soundId = howl.play();
const secondSoundId = howl.play();

const isPlayingSound1 = howl.playing(soundId);
// pause first sound
howl.pause(soundId);
howl.play(soundId);
// similarly a sprite's name can be passed into play/pause/etc.

Currently, the lib doesn't support this type of behavior, but i think it could be updated to do so without breaking changes. Thoughts?

I've played around with this locally, but it doesn't seems to work. I mean, I've made some changes so the play function will return the id of the audio, but if I tried to pause it, it just simply restarted the audio. That's why I'm thinking it's a howler.js issue.

I've made a workaround for my use case, and it doesn't looks like someone else missed this functionality, so IMO it's not worth to implement returning the ids.

brianshano commented 1 year ago

I've noticed too that initialVolume does not work when you load a track

E-Kuerschner commented 1 year ago

hey all the initial state load arguments have been fixed for version 2.0.1! Closing this issue