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

Replace Buffer with Uint8Array using DataView for encoding and decoding #246

Merged
merged 1 commit into from
Jul 16, 2021

Conversation

Borewit
Copy link
Owner

@Borewit Borewit commented Feb 28, 2021

Implements #245

Related: Borewit/tokenizer-token#4, Borewit/strtok3#522, Borewit/peek-readable#330

@Borewit Borewit self-assigned this Feb 28, 2021
@Borewit Borewit linked an issue Feb 28, 2021 that may be closed by this pull request
@Borewit Borewit mentioned this pull request Feb 28, 2021
lib/index.ts Outdated Show resolved Hide resolved
lib/index.ts Outdated Show resolved Hide resolved
Copy link

@jimmywarting jimmywarting left a comment

Choose a reason for hiding this comment

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

Good job, don't have so much to say. Did not expect a PR/Fix this fast.
Maybe requires a new major release now?

...Since it returns BigInt for 64 bit values now instead of a number - it's possible to cast the BigInt into a number by doing something like Number(3n) but i guess that ppl that want 64 values wants to work with bigint rather

  • you made a TextDecoder a requirement now which is good for not relying on Buffer

@Borewit
Copy link
Owner Author

Borewit commented Mar 1, 2021

Thanks for your review @jimmywarting

If I release this, it will be major release for sure.

I started to experiment with strtok3 & music-metadata offline with these changes, and it quite a challenge to adapt to these changes.

  • Lack of UintArray copy function (combination of Uint8Array.slice and Uint8Array.set can do the trick)
  • TextDecoder is not supporting the same encoding types as Buffer,: 'binary', 'ascii', 'ucs-2'
  • Lack of toString('hex'')

There is quite a bit of work left to do, and I need to be careful check if it worth the changing. Thing do not seems run faster from these changes, but I done proper measurement yet.

@jimmywarting
Copy link

Thing do not seems run faster from these changes, but I done proper measurement yet.

Buffer might be faster or equally fast as dataview in node but the browser polyfill maybe isn't as fast... Not entirely sure. But at least you won't need the hole buffer module in browser

@Borewit
Copy link
Owner Author

Borewit commented Mar 2, 2021

The vast majority of music-metadata users are Node.js users: ~ 75k vs 5k download/months, or 4544 vs 177 dependent projects if you prefer. If I can do something for the browser users that is great, sure, but I will never satisfy most of the potential users, as many of the user will not use this library because they think it to big. Therefor I would like to avoid a quality decrease for Node.js users.

For file-type users a small difference in performance is probably less very noticeable.

@Borewit Borewit force-pushed the dataview-uint8array-bigint branch from 924c9ef to deb5cd1 Compare June 28, 2021 18:01
@Borewit Borewit mentioned this pull request Jun 28, 2021
@Borewit Borewit force-pushed the dataview-uint8array-bigint branch from deb5cd1 to d1e7e3e Compare July 11, 2021 10:42
@Borewit
Copy link
Owner Author

Borewit commented Jul 11, 2021

Rebased this PR.
The migration to BigInt has now taked out of this PR and is covered in #300 (already merged).

@Borewit Borewit force-pushed the dataview-uint8array-bigint branch 2 times, most recently from 6c1057c to d87bbc5 Compare July 11, 2021 13:45
@Borewit
Copy link
Owner Author

Borewit commented Jul 11, 2021

Thanks for your review @jimmywarting.

I am trying to migrate token-types, strtok3, @tokenizer/token, peak-readable, file-type and music-metadata toward more recent general purpose API definitions.

Full dependency tree: npmgraph.js.org

Lot's of challenges so step by step.

@Borewit Borewit marked this pull request as draft July 11, 2021 19:17
@jimmywarting
Copy link

jimmywarting commented Jul 11, 2021

Nice readable graph, how did u get it?
I have used https://npm.anvaka.com/#/view/2d/music-metadata a bit to scatter around often where i can cut of some old packages by updating them to newer syntax or looking at which package i can cut of and replace with something else to reduce the # of dependencies


I could offer to help but i literally hate typescript syntax, wasting time compiling, getting garbage output and not following the standard. I like the typing doe, that why i use typescript only with a combination of vanilla js + checkjs enabled

I have this settings on in my vscode that pretty much turns any js project into a ts project without the need for the weird syntax and wasting time to compile - that way i can always be up to speed with latest javascript features

    "javascript.suggest.completeFunctionCalls": true,
    "javascript.suggest.completeJSDocs": true,
    "javascript.suggest.autoImports": true,
    "js/ts.implicitProjectConfig.checkJs": true,

@Borewit
Copy link
Owner Author

Borewit commented Jul 11, 2021

npmgraph.js.org provided the nice dependency chart. Did some post processing to keep the relevant packages. (I should have cut out the content-type as well, not important in this context).

I am using IntelliJ myself, as I am doing Java development as well. Can probably do the same.

I have pretty much all my projects written in TypeScript, at the time a class declaration was not understood by some browser, that kind of enabled me to write JavaScript coming from C# & Java. Maybe you are right and it served its purpose by now.

Your help is very much appreciated @jimmywarting.

One of the dependencies which should not be there is readable-web-to-node-stream, introduced by file-type. It is only required in the browser.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use dataview instead?
2 participants