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.

Sign up for GitHub

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

Jump to bottom

CID equality #208

Closed
achingbrain opened this issue Oct 15, 2022 · 2 comments · Fixed by #215
Closed

CID equality #208

achingbrain opened this issue Oct 15, 2022 · 2 comments · Fixed by #215
Labels
released

Comments

@achingbrain
Copy link
Member

We compare CIDs in our tests in lots (and lots) of places. E.g:

assert.deepEqual(cid1, cid2)

With v10 these checks have started failing where one of these CIDs has been read out of a byte array and does not occupy the entire byte array because of the now enumerable byteOffset property, for example here, blocking ipld/js-dag-pb#59.
achingbrain added a commit that referenced this issue Oct 15, 2022
@achingbrain
feat!: remove .byteLength and .byteOffset properties from CID
92112b7 
From #203

> I would like to also address byteOffset, byteLength fields. I don't think they were a bad idea, it's just I was not allowed to complete implementation of that idea.

Given that the feature is incomplete and untested, remove it for
now and if it's required it can be implemented fully in the future.

Fixes #208

BREAKING CHANGE: The CID class no longer has `.byteOffset` and `.byteLength` properties - these can be accessed via the `.bytes` property instead
achingbrain added a commit that referenced this issue Oct 15, 2022
@achingbrain
feat!: remove .byteLength and .byteOffset properties from CID
141e0f2 
From #203

> I would like to also address byteOffset, byteLength fields. I don't think they were a bad idea, it's just I was not allowed to complete implementation of that idea.

Given that the feature is incomplete and untested, remove it for
now and if it's required it can be implemented fully in the future.

Fixes #208

BREAKING CHANGE: The CID class no longer has `.byteOffset` and `.byteLength` properties - these can be accessed via the `.bytes` property instead
This was referenced Oct 15, 2022
Closed
Merged
Closed
Closed
@Gozala
Copy link
Contributor

Gozala commented Oct 16, 2022

That probably can be easily fixed by turning these to getters that just return this.bytes.byteOffset / this.bytes.byteLength

js-multiformats/src/cid.js

Lines 81 to 85 in 0ec751a

// ArrayBufferView
/** @readonly */
this.byteOffset = bytes.byteOffset
/** @readonly */
this.byteLength = bytes.byteLength

@Gozala Gozala mentioned this issue Oct 16, 2022
Closed
achingbrain added a commit that referenced this issue Oct 17, 2022
@achingbrain
fix: convert byteOffset and byteLength to getters
2a890fa 
Fixes #208 by converting `.byteOffset` and `.byteLength` properties
to getters.

Supersedes #210
This was referenced Oct 17, 2022
Merged
Merged
Merged
Gozala added a commit that referenced this issue Oct 17, 2022
@Gozala
chore: add test for #208
c9e970f
@Gozala Gozala mentioned this issue Oct 17, 2022
Closed
Gozala added a commit that referenced this issue Oct 17, 2022
@achingbrain @Gozala
fix: convert byteOffset and byteLength to getters (#215)
4e09490 
* fix: convert byteOffset and byteLength to getters

Fixes #208 by converting `.byteOffset` and `.byteLength` properties
to getters.

Supersedes #210

* Apply suggestions from code review

* Update test/test-cid.spec.js

Co-authored-by: Irakli Gozalishvili <contact@gozala.io>
github-actions bot pushed a commit that referenced this issue Oct 17, 2022
@semantic-release-bot
chore(release): 10.0.1 [skip ci]
b00bb45 
## [10.0.1](v10.0.0...v10.0.1) (2022-10-17)

### Bug Fixes

* convert byteOffset and byteLength to getters ([#215](#215)) ([4e09490](4e09490)), closes [#208](#208) [#210](#210)
@github-actions
Copy link

🎉 This issue has been resolved in version 10.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
2 participants
@Gozala @achingbrain