nikhilm / node-taglib

Simple taglib bindings to Javascript using node.js
http://nikhilm.github.com/node-taglib
MIT License
147 stars 25 forks source link

Fix compatibility with recent Node versions #63

Open Vortex375 opened 8 years ago

Vortex375 commented 8 years ago

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. :-)

nikhilm commented 8 years ago

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

Vortex375 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 https://github.com/nikhilm/node-taglib/pull/63#issuecomment-201926161

nikhilm commented 8 years ago

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 https://github.com/nikhilm/node-taglib/blob/80189d19a1da8ea45f9cbc90f19c2adc9f3685da/spec/taglibSpec.js#L11