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: use slash as flag that an object is a CID #217

Merged
merged 2 commits into from
Oct 19, 2022

Conversation

achingbrain
Copy link
Member

As per #212 making asCID enumerable breaks tests where modules don't handle self-referential data properly.

As proposed in #213 this swaps cid.CID === cid for cid['/'] === cid.bytes as a mechanism to tell consumers that the object in question is a CID which lets them write CBOR with the correct tags, for example.

Fixes #212
Closes #213

As per #212 making
`asCID` enumerable breaks tests where modules don't handle self-referential
data properly.

As proposed in #213
this swaps `cid.CID === cid` for `cid['/'] === cid.bytes` as a mechanism
to tell consumers that the object in question is a `CID` which lets
them write CBOR with the correct tags, for example.

Fixes #212
Closes #213
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.

works for me (although I'm being slowly worn down over these and it's a bit disheartening to see all of the historical compatibility cruft building up in these)

Copy link
Contributor

@Gozala Gozala left a comment

Choose a reason for hiding this comment

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

Provided some feedback on this PR. With those comments addressed, I'm fine with these changes. That said I would prefer to spend a bit more time discussing / considering alternatives.

src/cid.js Outdated Show resolved Hide resolved
src/cid.js Show resolved Hide resolved
sender.close()
receiver.close()
assert.strictEqual(cid2['/'], cid2.bytes)
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved the assertion to the end so if it fails, we still shut the message channel down and the process doesn't hang forever.

@achingbrain
Copy link
Member Author

@Gozala all done I think!

@achingbrain achingbrain merged commit 1cec619 into master Oct 19, 2022
@achingbrain achingbrain deleted the fix/use-slash-as-flag branch October 19, 2022 08:36
github-actions bot pushed a commit that referenced this pull request Oct 19, 2022
## [10.0.2](v10.0.1...v10.0.2) (2022-10-19)

### Bug Fixes

* use slash as flag that an object is a CID ([#217](#217)) ([1cec619](1cec619)), closes [#212](#212) [#213](#213)

### Trivial Changes

* **no-release:** rename varint test file so it is run ([#209](#209)) ([e32fe47](e32fe47))
* remove unnecessary dev deps ([#218](#218)) ([a43ffff](a43ffff))
@github-actions
Copy link

🎉 This PR is included in version 10.0.2 🎉

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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider alternative to asCID signaling mechanism Making asCID enumerable brakes assert functions in tests
3 participants