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

fix: make clean Uint8Array #1

Merged
merged 1 commit into from
Jun 5, 2020
Merged

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented May 14, 2020

See https://github.com/browserify/commonjs-assert/blob/v1.5.0/assert.js#L293-L294 for an example of why this can fail, the slice doesn't remove from the backing buffer. Yes that comparison is wrong but it's the version of assert that we're stuck with in Webpack without explicitly pulling in a newer version.

I'm also considering if maybe we shouldn't be doing a Uint8Array.from(o) in the coerce() function just to be sure.

@rvagg
Copy link
Member Author

rvagg commented May 14, 2020

fwiw I can't get some CID-related tests to pass in the browser because of this, they pass in Node.js but not in the browser and I'm not sure whether it's to do with borc preferring Uint8Array's in the browser but handling Buffers on Node.js (might even come from an fs call), or the comparison algorithm differences in assert that I get in the browser vs Node.js. Either way, it's going to bite other people.

Copy link
Member

@vmx vmx left a comment

Choose a reason for hiding this comment

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

CI is failing weirdly and still running on Node,js 10.

@rvagg
Copy link
Member Author

rvagg commented May 16, 2020

Yeah, CI should be ignored for now, this is still using the .travis.yml from js-ipld-dag-cbor, I imagine Mikeal will pull in his standard Github Actions workflows which do test and publish. This PR is against his WIP branch which isn't quite ready for merging into master, although with this patch it's working for me in js-datastore-car.

@mikeal mikeal merged commit 04b9710 into multiformats Jun 5, 2020
@mikeal
Copy link
Contributor

mikeal commented Jun 5, 2020

tests are passing but coverage is still not 100% which is where it needs to be before i turn on the automated releases.

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