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

Fix compatibility with recent Node versions #63

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Vortex375
Copy link

I rewrote large parts of your wrapper using the nan abstraction layer. I'm now able to compile and run it on node versions 4.x and 5.x. :-)


//baton->callback.Dispose();
Copy link
Owner

Choose a reason for hiding this comment

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

How does the reference to the callback get released when using NaN without this dispose call?

@nikhilm
Copy link
Owner

nikhilm commented Mar 24, 2016

Thanks for the patch @Vortex375! This looks great. Could you answer the dispose question before I merge this?

@Vortex375
Copy link
Author

Oops, I actually forgot about that one.

Should be fixed now. The NaN documentation states:

Existing handles can be disposed using an argument-less Nan::PersistentBase::Reset()

@nikhilm
Copy link
Owner

nikhilm commented Mar 24, 2016

Have you noticed this?

$ npm test          

> taglib@0.8.1 test /Users/nikhil/node-taglib
> vows --spec


  ♢ taglib bindings: Buffers

  tagSync metadata from mp3 buffer
    ✓ title should be A bit-bucket full of tags
    ✓ artist should be by gitzer's
    ✓ album should be on Waffles for free!
    ✓ track should be the first
    ✓ should be from 2011
    ✓ should have a silly comment
  tagSync data from a buffer with unknown format
    ✓ should raise an error
  tagSync data from a buffer with wrong format
    ✓ should raise an error
  tagSync data from empty buffer
    ✓ should lead to empty tags
  writing to a tag from a buffer
    ✓ should fail
FATAL ERROR: v8::HandleScope::CreateHandle() Cannot create a handle without a HandleScope

@Vortex375
Copy link
Author

Hmm, I'm trying to investigate this but I'm not sure where exactly the test crashes. If I do the saveSync() manually (also reading from a buffer) it does not crash...

There are also more problems: you can crash it by doing new taglib.Tag(), then trying to access any of the tag's properties. Tag is not supposed to be instantiable.

Was there a reason to export the Tag object?

@nikhilm
Copy link
Owner

nikhilm commented Mar 30, 2016

Sorry, I forgot about this. I'll try to reproduce this on Thursday ideally.
The problem might have to do with providing a default constructor or
similar that throws an error when one attempts to instantiate Tag.

On Sat, Mar 26, 2016 at 1:24 PM, Benjamin Schmitz notifications@github.com
wrote:

Hmm, I'm trying to investigate this but I'm not sure where exactly the
test crashes. If I do the saveSync() manually (also reading from a
buffer) it does not crash...

There are also more problems: you can crash it by doing new taglib.Tag(),
then trying to access any of the tag's properties. Tag is not supposed to
be instantiable.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#63 (comment)

@nikhilm
Copy link
Owner

nikhilm commented Oct 7, 2016

I am sorry, I haven't been working on this at all. The Tag object does not need to be exported into JS scope. I would be ok with it being hidden. I believe it was only exported for

assert.equal(Taglib.Tag, tag.constructor);

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