Borewit / music-metadata

Stream and file based music metadata parser for node. Supporting a wide range of audio and tag formats.
MIT License
868 stars 89 forks source link

feat: convert most Buffer usage to Uint8Array #2103

Open bjornstar opened 6 days ago

bjornstar commented 6 days ago

As you mentioned in https://github.com/Borewit/token-types/pull/650 a good test is if we can use token-types (& file-type) in music-metadata.

This is not exhaustive, there are still some Buffer usages remaining. However I have all the tests passing which I hope is good enough for a first pass.

I also discovered a race condition with the file-type parsing, we need to wait until the file type is parsed before we add the tags.

This PR depends on changes to:

Borewit commented 6 days ago

I propose the primary objective (requirement) is to ensure music-metadata can depend on strtok3 and token-type, the "Goodbye Node.js Buffer" versions.

To replace remaining (all) Buffer dependencies in music-metadata is a secondary objective, which can be address in following stage (different PR).

The first objective, I think it is good to do before applying the changes in file-type (as health check as proposed here). The remaining Buffer dependencies in music-metadata should not delay your effort to work towards getting this into file-type.

Only the secondary stage depends on the "Goodbye Node.js Buffer" version of file-type.

bjornstar commented 5 days ago

I propose the primary objective (requirement) is to ensure music-metadata can depend on strtok3 and token-type, the "Goodbye Node.js Buffer" versions.

To replace remaining (all) Buffer dependencies in music-metadata is a secondary objective, which can be address in following stage (different PR).

The first objective, I think it is good to do before applying the changes in file-type (as health check as proposed here). The remaining Buffer dependencies in music-metadata should not delay your effort to work towards getting this into file-type.

Only the secondary stage depends on the "Goodbye Node.js Buffer" version of file-type.

I agree, we don't need to replace all the Buffer usage in the first pass. I think file-type can go first, but it should probably work in either order.

Once the shared dependencies are released it should be clear that file-type and music-metadata both work with the new versions.

Borewit commented 5 days ago

Can you have a look to the remaining test cases, why these are failing?

Borewit commented 5 days ago

I managed to music-metadata pass all unit tests 💯✅, combining all outstanding PR's:

bjornstar commented 5 days ago

Appveyor is failing because uint8array-extras specifies node 18 as the minimum node version.

I added a fix for it in #2105

bjornstar commented 8 hours ago

@Borewit This needs a new release of strtok3 that includes https://github.com/Borewit/strtok3/pull/1074