Skip to content

Conversation

@dariocravero
Copy link

Hi,

I was about to write exactly this library (and probably some of its ecosystem) until I found yours :).
Great work standardising the weird bits under the hood!

I'd love it if it would be slimmer though, and I think it's totally doable :).
This PR cuts down almost half the size by implementing fetch instead of xhr and by using Promise instead of EventEmitter.
Events are gone but a communication channel can be introduced as an option if truly needed.

I'd love to hear your thoughts about this.
Thanks!
Darío

@mattdesl
Copy link
Contributor

Hey! Glad you are interested in this project. 😄 Thanks for taking the time to explore the code.

This PR removes a lot functionality from the library and introduces a peer dependency on Promise and Fetch. This means it does not work in many browsers without polyfills like es6-promise and whatwg-fetch, so it introduces more work on the end-user to get the module working and also introduces some bundle bloat from those polyfills. The final bundle size after polyfills is likely going to be similar, maybe even larger, than it is currently.

The xhr, object-assign and events modules are pretty thin and tend to be found in many other areas of npm, so if you are building an application with modules it's likely you will have them in your bundle already (i.e. they can be de-duplicated). Also, some of these are in place for good measure, such as object-assign being used to avoid mutating the user's passed in options.

The good thing is that I am building a lot of this in separate modules (as you can see with detect-audio-autoplay and all the other modules), so even if we disagree on the architecture/design of the core "player" module, there is probably some common ground to be found in the lower-level utilities! 😄

I will fix up the listed dependencies, thanks for pointing that out. 🍺

Cheers.

@mattdesl
Copy link
Contributor

Another point against promises in this case - sometimes you will want to use the audioPlayer instance immediately after creation, even before the audio has finished loading.

const audioPlayer = webAudioPlayer('blah.mp3')
if (audioPlayer.element) audioPlayer.element.addEventListener('timeupdate', ...)

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

Successfully merging this pull request may close these issues.

2 participants