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

2.0 API proposal #42

Open
dy opened this issue Jun 16, 2017 · 13 comments
Open

2.0 API proposal #42

dy opened this issue Jun 16, 2017 · 13 comments
Assignees

Comments

@dy
Copy link
Member

dy commented Jun 16, 2017

Ideally API should be

const Speaker = require('audio-speaker')
let write = Speaker(options)
write(audioBuffer|data, callback?)

Stream counterparts are audio-speaker-stream and pull-audio-speaker

@dy dy mentioned this issue Jun 16, 2017
15 tasks
@vectrixdevelops
Copy link

vectrixdevelops commented Jun 16, 2017

The issue regarding the proposal for 2.0 is as follows.

We can either run with the current version of audio-mpg123 which is basically ready for release, just need to split up the projects of course.

Or we could wait until the NAPI version of audio-mpg123 is complete. Then make changes the API to follow the changes in audio-mpg123 then release.

Although the issue with releasing now is, the changes that will come with updating to the NAPI audio-mpg123 would require us to modify the API a bit. I suggest we could deprecate the methods and add newer methods for it that work better with ArrayBuffers for instance.

The issue with waiting for the NAPI version is that there would be a fairly long wait until NAPI is stable and ready to be used on production. To use NAPI modules they require the NAPI flag to add the dependency. Which would strike as an issue when installing audio-speaker. They will most likely remove this flag later on when it is stable.

So I need opinions on what way we should take this. @dfcreative @jamen 😄

@jamen
Copy link
Contributor

jamen commented Jun 16, 2017

Hey. So I'm all up for using nan instead of node-addon-api if we can get something npm install-able out there faster. My only concern is I just don't want it to feel like a wasted effort, because NAPI may not be unstable for that long, and there is still things to do on the NAPI branch and here in the meantime that could close the gap.

I'll let you make the decision on where to invest your time, though.

@dy
Copy link
Member Author

dy commented Jun 17, 2017

As for me I would choose getting things done rather than perfectly undone. That is if we can release this API within one-day effort but with nan that is perfectly fine, rather than if we start using node-addon-api and get stuck for months due to some unimplemented/unstable feature.

But we have always to be open to refactoring, ie. breaking API later to allow for node-addon-api is a good change.

@vectrixdevelops
Copy link

Cool, I'll get it released for nan then.

@vectrixdevelops
Copy link

Do we still want the browser implementation in 2.0? @dfcreative @jamen

@dy
Copy link
Member Author

dy commented Jun 21, 2017

@connorhartley yes, but I can take care of it

@vectrixdevelops
Copy link

@dfcreative If you would like to make some tests for it that would be sweet! I am working in the release-2.0-browser branch, should be nearly ready. 😄

@dy
Copy link
Member Author

dy commented Jun 21, 2017

@connorhartley I've added minimal easy-peasy test for browser and covered browser implementation here 944f254.
Working on audio-oscillator by your request now.

@vectrixdevelops
Copy link

Thank you very much @dfcreative !

@iwasrobbed
Copy link

Any chance I can test this out? I'm building an Electron app w/ node for streaming Amazon Polly speech mp3's

@jamen
Copy link
Contributor

jamen commented Jul 31, 2017

@iwasrobbed That sounds awesome, feel free to keep us updated!

There is a pull request open at #44 where most of this is implemented already, if I'm not mistaken. I think the concern at the moment is how stable #44 actually is, I think we need more extensive testing, but I'm unable to check this at the moment (more could have been added since I've looked at it)

@connorhartley @dfcreative, what else needs to be done to wrap this up? 😄

(P.S., if you are decoding with audio.js too, you might also want to see audiojs/audio-decode#2)

@iwasrobbed
Copy link

iwasrobbed commented Jul 31, 2017

Thanks @jamen ; I gave it a quick test drive (Mac OS X Sierra), but doesn't seem quite ready yet:

Uncaught Error: Cannot find module '/Users/rob/blah/node_modules/audio-mpg123/lib/audio_mpg123.node'

Let me know if I can help test when you're ready and happy to do so

@vectrixdevelops
Copy link

vectrixdevelops commented Aug 1, 2017

@iwasrobbed
We haven't added a prebuilt macos version to the releases as we have been struggling to find someone with macos to help us build it. Did it attempt to build it manually? Were there any errors involved with compiling the binding? If this continues, do you think you could open a separate issue for this? Thanks for your help. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants