Skip to content
This repository has been archived by the owner on Apr 29, 2020. It is now read-only.

feat: adds js implementation of rabin chunker for windows and browser #30

Merged
merged 1 commit into from
May 24, 2019

Conversation

achingbrain
Copy link
Collaborator

@achingbrain achingbrain requested a review from alanshaw May 24, 2019 20:16
@achingbrain
Copy link
Collaborator Author

Also fixes a bug with the native chunker that meant it was truncating files.

@achingbrain achingbrain merged commit 542b3e4 into master May 24, 2019
@achingbrain achingbrain deleted the add-js-rabin branch May 24, 2019 20:21
Copy link
Contributor

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

This is really cool ❤️🚀

🙏 please next time can the @ipfs/wg-js-core have a chance to review before merge+release?


const jsRabin = () => {
// see https://github.com/datproject/rabin/blob/c0378395dc0a125ab21ac176ec504f9995b34e62/src/rabin.cc
class Rabin {
Copy link
Contributor

Choose a reason for hiding this comment

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

npm i rabin.js?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean rabinjs? It has no repo on npm and only 5 downloads a week.

Not sure it's ready for production.

maxChunkSize: KiB256 + (KiB256 / 2),
window: 16,
polynomial: '0x3DF305DFB2A805'
maxChunkSize: KiB256 + (KiB256 / 2)
}

const chunks = await all(chunker([file], opts))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a test to check that the output of JS rabin impl is the same as native rabin impl?

Copy link
Collaborator Author

@achingbrain achingbrain May 31, 2019

Choose a reason for hiding this comment

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

The rabin tests run in node and the browser, one of them tests the size of chunks emitted so I thought that was sufficient.

I was adding interop tests for more comprehensive testing but then @hugomrdias compiled the C version to WASM in rabin-wasm so may wait for that to be ready.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interop tests are at ipfs/interop#74 - though as ever the build is failing due to unrelated tests that depend on networking.

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

Successfully merging this pull request may close these issues.

2 participants