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

fix: convert byteOffset and byteLength to getters #215

Merged
merged 3 commits into from
Oct 17, 2022
Merged

fix: convert byteOffset and byteLength to getters #215

merged 3 commits into from
Oct 17, 2022

Conversation

achingbrain
Copy link
Member

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

Supersedes #210

@achingbrain
fix: convert byteOffset and byteLength to getters
2a890fa 
Fixes #208 by converting `.byteOffset` and `.byteLength` properties
to getters.

Supersedes #210
@achingbrain achingbrain mentioned this pull request Oct 17, 2022
Closed
@achingbrain achingbrain mentioned this pull request Oct 17, 2022
Merged
rvagg
rvagg approved these changes Oct 17, 2022
View reviewed changes
Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

sgtm but I'll leave it to @Gozala to weigh in; I'm a bit tired to having to deal with these myself, I think the intention was to have an ArrayBuffer that could also be a CID itself (kind of like the Cid type in Go which is just a wrapped byte string) but we've not really evolved beyond just having a concrete class—does the new interface (and/or the newly proposed ones) get us any closer? is it worth the bother holding on to these as important properties?

@achingbrain
Copy link
Member Author

@rvagg this was @Gozala's suggestion 😜

does the new interface (and/or the newly proposed ones) get us any closer?

Most of them remove these fields so no, not exactly.

Is it worth the bother holding on to these as important properties?

I don't think so, no. Can restore #210 if that's a better solution.

@achingbrain
Copy link
Member Author

achingbrain commented Oct 17, 2022 •
edited
Loading

@Gozala Happy for this go in to unblock the roll out of multiformats v10 to @ipld/dag-*, ipfs, etc?

@achingbrain achingbrain mentioned this pull request Oct 17, 2022
Merged
RangerMauve
RangerMauve approved these changes Oct 17, 2022
View reviewed changes
Copy link
Collaborator

@RangerMauve RangerMauve left a comment

Choose a reason for hiding this comment

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

LGTM, might as well wait for gozala tho

@Gozala
Copy link
Contributor

Gozala commented Oct 17, 2022

Ah sorry @achingbrain I have concurrently submitted #216. I'm happy with either, that said mine takes care of coverage issues reported in this pr through this change 16d6ef7

Gozala
Gozala reviewed Oct 17, 2022
View reviewed changes
src/cid.js Outdated Show resolved Hide resolved
@Gozala Gozala mentioned this pull request Oct 17, 2022
Closed
Gozala
Gozala reviewed Oct 17, 2022
View reviewed changes
test/test-cid.spec.js Outdated Show resolved Hide resolved
@Gozala Gozala merged commit 4e09490 into master Oct 17, 2022
@Gozala Gozala deleted the fix/array-buffer-props-as-getters branch October 17, 2022 21:44
github-actions bot pushed a commit that referenced this pull request 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 PR is included in version 10.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Gozala Gozala mentioned this pull request Oct 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RangerMauve RangerMauve RangerMauve approved these changes

@MarcoPolo MarcoPolo MarcoPolo approved these changes

@rvagg rvagg rvagg approved these changes

@Gozala Gozala Gozala approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CID equality
5 participants
@achingbrain @Gozala @rvagg @MarcoPolo @RangerMauve