Skip to content
This repository has been archived by the owner on Oct 9, 2021. It is now read-only.

N-API #10

Closed
wants to merge 6 commits into from
Closed

N-API #10

wants to merge 6 commits into from

Conversation

jamen
Copy link
Contributor

@jamen jamen commented Jun 7, 2017

A work in progress branch of N-API refactor

  • mpg123.create
  • mpg123.open
  • mpg123.close
  • mpg123.write
  • mpg123.flush
  • mpg123.<S*,U*,F*> formats
  • napi_* status checks

It includes some breaking API changes that take a more simple approach. e.g.

  1. There is no more finis(success) callbacks, we just return booleans or objects.
  2. The pointer is a plain object using napi_create_external instead of the old pointer_wrapper.h
  3. The formats are shorter, at mpg123.<S8,S16,S32,U8,U16,U32,F32,F64>, for signed, unsigned, and floats

Currently working on mpg123.write and mpg123.flush. Will be tricky because I have to learn their async system! I am not that experienced with C++ so we will see how this goes, might take a bit longer. Help wanted.

@vectrixdevelops
Copy link
Collaborator

This could be merged after NAPI reaches a more mature and stable state in Node. Currently master and NAPI can be used as a test comparison to assist NAPI with development too. Keep up the good work!

test.js Outdated
)
})

test('opens mpg123', function (t) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason the old test never checked if the binding could open, was because the test was designed for Travis during audio-speaker tests. Travis doesn't have the appropriate environment to open an instance of mpg123. If you could test this, try find a way around it or make another test for Travis (add it to the commands Travis will run). That would be great!

@jamen
Copy link
Contributor Author

jamen commented Jun 10, 2017

Not bothered with Travis atm cuz I dont even have teste written. Also mpg123.write still needs work, still learning how to do their async.

@vectrixdevelops
Copy link
Collaborator

Just for when you get around to it.

@dy
Copy link

dy commented Jan 30, 2018

Hey @jamen what's the status of the issue? How do you think is the NAPI strong enough to go on with that instead of nan?

@jamen
Copy link
Contributor Author

jamen commented Jan 30, 2018

Hey @dfcreative. I pushed some changes and I'm gonna keep hacking at this branch. Replied to your email for more details.

About the stability of N-API. I think it is stable enough. It is no longer hidden behind a flag, but the API did change a bit since I last used it (nothing really complicated). One downside is that it outputs this:

(node:24955) Warning: N-API is an experimental feature and could change at any time.

Maybe there is a flag we can compile with in binding.gyp to disable this?

@jamen
Copy link
Contributor Author

jamen commented Feb 8, 2018

I'll be busy next few weeks. Here are resources I've used if anyone is interested in continuing this:

Some extra thoughts:

  • libao might be a good alternative. When I tested it would fail because I didn't have alsa-dev for Arch installed, but I didn't figure that out until after changing libraries.
  • libsoundio seems really challenging to bind due to how it uses callbacks. It seems friendlier for a C++ user but seemingly doesn't play well with N-API.
  • Since we are publishing prebuilds I don't think it matters which libraries we end up using, although it would make it easier or harder for us to maintain.

@jamen
Copy link
Contributor Author

jamen commented Apr 24, 2018

N-API is no longer experimental in Node v10

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

Successfully merging this pull request may close these issues.

3 participants