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

Making asCID enumerable brakes assert functions in tests #212

Closed
Gozala opened this issue Oct 15, 2022 · 3 comments · Fixed by #217
Closed

Making asCID enumerable brakes assert functions in tests #212

Gozala opened this issue Oct 15, 2022 · 3 comments · Fixed by #217
Labels

Comments

@Gozala
Copy link
Contributor

Gozala commented Oct 15, 2022

After updating to v10 lot of our tests started fail, as it turns out problem is introduced by a asCID property becoming enumerable. Most assert functions seem to get tripped on link.asCID field either crashing with stack overflow or simply deciding cids aren't equal 😭

I'm not sure what the fix is here, but it's kind of a pain to deal with now.

@achingbrain
Copy link
Member

See also #208 - when decoding CIDs from the return value of Uint8Array.subarray, it can cause byteOffset to be different between two CID instances that otherwise have the same version, code and multihash so deep equality tests fail where they previously passed.

@Gozala
Copy link
Contributor Author

Gozala commented Oct 17, 2022

Turns out that chai deepEqual assert handles asCID fine because it does proper memoization during assertions, we have not run into #208 because we don't really use CIDv0 anywhere and that only can occur with v0 CIDs as per #216 & either way that can be resolved by that PR.

Our problems seem to be coming from https://www.npmjs.com/package/chai-subset package which does not do any memoization and consequently it fails with stack overflow. And we have to to use chai-subset because chai deepNestedInclude has a strange behavior with arrays.

I think our our options are to either:

  1. Fix chai-subset and leave asCID as is, because if all other properties are deepEqual values should be considered deep equal.
  2. Switch to alternative signalling described in Trim Link / CID interface and introduce something like LinkView interface #214 which boils down to using cid['/'] === cid.bytes instead of cid.asCID === this, which would work equally well, but hopefully will cause far less problems with third party code that does not handle self-referential data (although if I recall correctly @mikeal intentionally wanted to break code that naively attempted to treat CID as something else, but maybe his position shifted given more data)

achingbrain added a commit that referenced this issue Oct 18, 2022
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
achingbrain added a commit that referenced this issue Oct 18, 2022
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
github-actions bot pushed a commit that referenced this issue 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 issue has been resolved 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
2 participants