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

Convert most Buffer usage to Uint8Array #2103

Merged
merged 21 commits into from
Jul 8, 2024
Merged

Convert most Buffer usage to Uint8Array #2103

merged 21 commits into from
Jul 8, 2024

Conversation

bjornstar
Copy link
Contributor

@bjornstar bjornstar commented Jun 28, 2024

As you mentioned in Borewit/token-types#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
Copy link
Owner

Borewit commented Jun 29, 2024

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
Copy link
Contributor Author

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 Borewit self-requested a review June 29, 2024 14:45
@Borewit
Copy link
Owner

Borewit commented Jun 29, 2024

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

@Borewit
Copy link
Owner

Borewit commented Jun 29, 2024

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

@bjornstar
Copy link
Contributor Author

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

I added a fix for it in #2105

@bjornstar
Copy link
Contributor Author

@Borewit This needs a new release of strtok3 that includes Borewit/strtok3#1074

@coveralls
Copy link

Coverage Status

coverage: 96.524% (-0.001%) from 96.525%
when pulling 9f7b8d1 on bjornstar:goodbye-nodejs-buffer
into 24a507e on Borewit:master.

@coveralls
Copy link

Coverage Status

coverage: 96.524% (-0.001%) from 96.525%
when pulling a7a45ff on bjornstar:goodbye-nodejs-buffer
into 24a507e on Borewit:master.

@bjornstar bjornstar marked this pull request as ready for review July 5, 2024 21:05
@coveralls
Copy link

Coverage Status

coverage: 96.523% (-0.002%) from 96.525%
when pulling 4503c55 on bjornstar:goodbye-nodejs-buffer
into 24a507e on Borewit:master.

@bjornstar
Copy link
Contributor Author

All dependencies have been updated, this should be good to go.

@coveralls
Copy link

Coverage Status

coverage: 96.514% (-0.01%) from 96.525%
when pulling e767904 on bjornstar:goodbye-nodejs-buffer
into 24a507e on Borewit:master.

@Borewit
Copy link
Owner

Borewit commented Jul 7, 2024

All dependencies have been updated, this should be good to go.

Sorry for not being more responsive, my time is limited and I first objective is to finalize sindresorhus/file-type#635, aiming release no Node.js dependent for file-type using the primary (non Node) entry point.

@coveralls
Copy link

coveralls commented Jul 8, 2024

Coverage Status

coverage: 96.514% (-0.01%) from 96.525%
when pulling cda25d5 on bjornstar:goodbye-nodejs-buffer
into 06d9425 on Borewit:master.

Copy link
Owner

@Borewit Borewit left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your commitment and hard work @bjornstar !

@Borewit Borewit merged commit eb0f8e6 into Borewit:master Jul 8, 2024
16 of 17 checks passed
@bjornstar bjornstar deleted the goodbye-nodejs-buffer branch July 9, 2024 08:11
@Borewit Borewit changed the title feat: convert most Buffer usage to Uint8Array Convert most Buffer usage to Uint8Array Jul 9, 2024
@Borewit Borewit added the improvement Improvement of existing functionality label Jul 9, 2024
@Borewit
Copy link
Owner

Borewit commented Jul 10, 2024

Part of v9.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants