node-taglib icon indicating copy to clipboard operation
node-taglib copied to clipboard

Fix compatibility with recent Node versions

Open Vortex375 opened this issue 8 years ago • 6 comments

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

Vortex375 avatar Mar 18 '16 15:03 Vortex375

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

nikhilm avatar Mar 24 '16 05:03 nikhilm

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()

Vortex375 avatar Mar 24 '16 10:03 Vortex375

Have you noticed this?

$ npm test          

> [email protected] 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

nikhilm avatar Mar 24 '16 16:03 nikhilm

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?

Vortex375 avatar Mar 26 '16 20:03 Vortex375

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 [email protected] 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 avatar Mar 30 '16 00:03 nikhilm

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

nikhilm avatar Oct 07 '16 19:10 nikhilm