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

Update to token-types 4.0.1 & strtok3 6.2.2 #466

Merged
merged 2 commits into from
Jul 22, 2021

Conversation

Borewit
Copy link
Collaborator

@Borewit Borewit commented Jul 20, 2021

Changes:

  1. Update to token-types 4.0.1 & strtok3 6.2.2
  2. The updated dependencies try to abstract Buffer dependency by using Uint8Array where possible.
  3. Remove Buffer as type, as Buffer is a sublcass of Uint8Array.
  4. Add @tokenizer/token to development dependencies. This was not required in the past as @tokenizer/token was incorrectly included indirectly as a normal dependency.

Dependent PR: Borewit/music-metadata#852, to align dependencies token-types & strtok3

Dependency on Buffer is still there, but in some dependent module the Buffer dependency has been entirely abstracted to Uint8Array. Let's say I try to get rid of Buffer dependencies if not really required.

By doing this, I hope on the long run I hope modules will be better suitable for multipurpose (Node.js & browser) usage.

Fixes:

@Borewit
Copy link
Collaborator Author

Borewit commented Jul 20, 2021

@jimmywarting, can you please review?

throw new TypeError(`Expected the \`input\` argument to be of type \`Uint8Array\` or \`Buffer\` or \`ArrayBuffer\`, got \`${typeof input}\``);
}

const buffer = input instanceof Buffer ? input : Buffer.from(input);
const buffer = input instanceof Uint8Array ? input : new Uint8Array(input);
Copy link
Contributor

Choose a reason for hiding this comment

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

Had to take a extra look at this, but then i realized that you guard yourself with type checking above.

Buffer.from(input) dose a little bit more magic than new Uint8array(input) like converting strings.
if this is something you also want to support, then a easy way to convert string to uint8 is via new TextEncoder().encode(str)
I just released a package that converts literally anything to Uint8Array with 0 copies if it's of any interest to you... it's called to-uint8array


The Buffer.isBuffer(input) would never be called cuz it would be covered by instanceof uint8 check

Copy link
Owner

Choose a reason for hiding this comment

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

like converting strings.

We wouldn't want that at least. The API is documented to accept a buffer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So we allow kind of 3 different input's: Buffer, Uint8Array, ArrayBuffer.
I don't think we need to explicitly add a Buffer here, as Uint8Array is sufficient.

As @sindresorhus indicated, string was not something we had in mind as an input.

Copy link
Contributor

@jimmywarting jimmywarting Jul 20, 2021

Choose a reason for hiding this comment

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

I didn't expect it to work with strings either.

Just wanted to recommend my package cuz it can convert any ArrayBufferView (including Buffer + DataView) & ArrayBuffer and turn them into uint8arrays without copying the memory
Quite handy for any typed array

The updated dependencies try to abstract Buffer dependency by using Uint8Array where possible.
Remove Buffer as type, as Buffer is an Uint8Array as well.
Add @tokenizer/token to development dependencies. This was not required in the past as
@tokenizer/token was incorrectly included indirectly as a normal dependency.
@Borewit Borewit force-pushed the update-strtok-6.2.2-token-types-4.0.1 branch from a4ef125 to 58cb0f3 Compare July 20, 2021 13:09
@Borewit Borewit self-assigned this Jul 20, 2021
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.

3 participants